-
Notifications
You must be signed in to change notification settings - Fork 390
[azure] fix bug causing log splitting to not work properly #950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -637,3 +637,184 @@ describe('HTTPClient', function() { | |
}); | ||
}); | ||
}); | ||
|
||
describe('Log Splitting', function() { | ||
function setUpWithLogSplittingConfig(config) { | ||
const forwarder = new client.EventhubLogHandler(fakeContext()); | ||
|
||
// Mock the log splitting configuration | ||
forwarder.logSplittingConfig = config; | ||
|
||
// Mock addTagsToJsonLog to set ddsource for testing | ||
forwarder.addTagsToJsonLog = x => { | ||
return Object.assign({ | ||
ddsource: 'azure.datafactory', | ||
ddsourcecategory: 'azure', | ||
service: 'azure', | ||
ddtags: 'forwardername:testFunctionName' | ||
}, x); | ||
}; | ||
|
||
forwarder.addTagsToStringLog = x => { | ||
return { | ||
ddsource: 'azure.datafactory', | ||
ddsourcecategory: 'azure', | ||
service: 'azure', | ||
ddtags: 'forwardername:testFunctionName', | ||
message: x | ||
}; | ||
}; | ||
|
||
return forwarder; | ||
} | ||
|
||
describe('#logSplitting with azure.datafactory configuration', function() { | ||
beforeEach(function() { | ||
this.testConfig = { | ||
'azure.datafactory': { | ||
paths: [['properties', 'Output', 'value']], | ||
keep_original_log: true, | ||
preserve_fields: false | ||
} | ||
}; | ||
this.forwarder = setUpWithLogSplittingConfig(this.testConfig); | ||
}); | ||
|
||
it('should split logs with correct field structure due', function() { | ||
const inputLog = { | ||
resourceId: '/subscriptions/test-sub/resourceGroups/test-rg/providers/Microsoft.DataFactory/factories/test-factory', | ||
properties: { | ||
Output: { | ||
value: [ | ||
{ id: 1, name: 'item1', status: 'success' }, | ||
{ id: 2, name: 'item2', status: 'failed' }, | ||
{ id: 3, name: 'item3', status: 'success' } | ||
] | ||
} | ||
}, | ||
timestamp: '2023-01-01T00:00:00Z', | ||
category: 'PipelineRuns' | ||
}; | ||
|
||
const result = this.forwarder.handleLogs(inputLog); | ||
|
||
assert.equal(result.length, 4); // 3 split logs + 1 original | ||
assert.equal(result[0].ddsource, 'azure.datafactory'); | ||
assert.ok(result[0].parsed_arrays); | ||
|
||
// the first 3 logs should be split from the array | ||
for (let i = 0; i < result.length -1; i++) { | ||
assert.equal(result[i].ddsource, 'azure.datafactory'); | ||
assert.ok(!Array.isArray(result[i].parsed_arrays.properties['Output']['value'])); | ||
} | ||
|
||
// The last log should be the original log | ||
assert.equal(result[3].ddsource, 'azure.datafactory'); | ||
assert.ok(Array.isArray(result[3].properties['Output']['value'])); | ||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
it('should preserve original log when path does not exist', function() { | ||
const inputLog = { | ||
properties: { | ||
SomeOtherField: 'value' | ||
} | ||
}; | ||
|
||
const result = this.forwarder.handleLogs(inputLog); | ||
|
||
// Should only have 1 log (original) since path doesn't exist | ||
assert.equal(result.length, 1); | ||
assert.equal(result[0].ddsource, 'azure.datafactory'); | ||
assert.equal(result[0].properties.SomeOtherField, 'value'); | ||
}); | ||
Comment on lines
+716
to
+729
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
it('should handle null/undefined values in path gracefully', function() { | ||
const inputLog = { | ||
properties: { | ||
Output: null | ||
} | ||
}; | ||
|
||
const result = this.forwarder.handleLogs(inputLog); | ||
|
||
// Should only have 1 log (original) since path leads to null | ||
assert.equal(result.length, 1); | ||
assert.equal(result[0].ddsource, 'azure.datafactory'); | ||
assert.equal(result[0].properties.Output, null); | ||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
describe('#logSplitting with non-datafactory source', function() { | ||
beforeEach(function() { | ||
this.testConfig = { | ||
'azure.datafactory': { | ||
paths: [['properties', 'Output', 'value']], | ||
keep_original_log: true, | ||
preserve_fields: true | ||
} | ||
}; | ||
this.forwarder = setUpWithLogSplittingConfig(this.testConfig); | ||
|
||
// Override to return different source | ||
this.forwarder.addTagsToJsonLog = x => { | ||
return Object.assign({ | ||
ddsource: 'azure.storage', | ||
ddsourcecategory: 'azure', | ||
service: 'azure', | ||
ddtags: 'forwardername:testFunctionName' | ||
}, x); | ||
}; | ||
}); | ||
|
||
it('should not split logs from other sources', function() { | ||
const inputLog = { | ||
properties: { | ||
Output: { | ||
value: [ | ||
{ id: 1, name: 'item1' }, | ||
{ id: 2, name: 'item2' } | ||
] | ||
} | ||
} | ||
}; | ||
|
||
const result = this.forwarder.handleLogs(inputLog); | ||
|
||
// Should only have 1 log (original) since source doesn't match config | ||
assert.equal(result.length, 1); | ||
assert.equal(result[0].ddsource, 'azure.storage'); | ||
assert.ok(!result[0].parsed_arrays); | ||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
describe('#findSplitRecords method behavior', function() { | ||
beforeEach(function() { | ||
this.forwarder = new client.EventhubLogHandler(fakeContext()); | ||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
it('should return an object with the field at the end of of the chain in fields', function() { | ||
const value = [1,2,3]; | ||
const fields = ['properties', 'Output', 'value']; | ||
const record = { | ||
properties: { | ||
Output: { | ||
value: value | ||
} | ||
} | ||
}; | ||
|
||
const result = this.forwarder.findSplitRecords(record, fields); | ||
assert.equal(result, value); | ||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
it('should return null when path leads to null/undefined', function() { | ||
const fields = ['properties', 'Output', 'value']; | ||
const record = { | ||
0: null | ||
}; | ||
|
||
const result = this.forwarder.findSplitRecords(record, fields); | ||
assert.equal(result, null); | ||
}); | ||
Comment on lines
+810
to
+818
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
mattsp1290 marked this conversation as resolved.
Show resolved
Hide resolved
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Code Quality Violation
Prefer object spread over `Object.assign` (...read more)
This rule encourages the use of the object spread syntax over the
Object.assign
method when creating a new object from an existing one where the first argument is an empty object. This is because the object spread syntax is more concise, easier to read, and can eliminate the need for null checks that are often necessary withObject.assign
.If you need to use
Object.assign
, make sure that the first argument is not an object literal, as this can easily be replaced with the spread syntax.