-
Notifications
You must be signed in to change notification settings - Fork 513
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
Allow uploading JSON-per-line OTLP data #2380
Conversation
OpenTelemetry specifies that JSON line files can contain valid JSON objects which are seperated with a newline character. The existing readJsonFile function couldn't handle those instances, this PR fixes that, along with test cases to verify. Signed-off-by: Muthukumar <muthuku37@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2380 +/- ##
=======================================
Coverage 96.60% 96.61%
=======================================
Files 254 254
Lines 7662 7679 +17
Branches 1994 1997 +3
=======================================
+ Hits 7402 7419 +17
Misses 260 260 ☔ View full report in Codecov by Sentry. |
OpenTelemetry specifies that JSON line files can contain valid JSON objects which are seperated with a newline character. The existing readJsonFile function couldn't handle those instances, this PR fixes that, along with test cases to verify. Signed-off-by: Muthukumar <muthuku37@gmail.com>
Signed-off-by: Muthukumar <muthuku37@gmail.com>
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.
lgtm, minor nits
Signed-off-by: Muthukumar <muthuku37@gmail.com>
Woops, I didn't lint the file after my last commit. I'll do it. |
I just pushed some clean-up |
cool |
when I run the test manually I am getting very odd result:
Here the test is marked as succeeded, yet an exception is logged indicating that the match condition was not met. I am also wondering how the |
If you try this sample test I wrote with plain node (just to emulate whats happening), all the lines get logged without any concern? The lines that sentry says is missing gets logged by the test function. It has got something to do with jest? I'll continue debugging. |
It has something to do with jest and fs module, I feel. I've been debugging for a while, however no success. Will look into this tomorrow. Additionally @yurishkuro , views on using mock functions from Jest? That way, we do not have to talk with the file system from the test file at all. I believe mock functions should solve the issue. |
Mock is used instead of transform function in test cases. Signed-off-by: Muthukumar <muthuku37@gmail.com>
@yurishkuro Review once again please, the test case file. Only the tests are changed, but hopefully now everything should work as expected. |
Thanks! |
OpenTelemetry specifies that JSON line files can contain valid JSON objects which are seperated with a newline character. The existing readJsonFile function couldn't handle those instances, this PR fixes that, along with test cases to verify.
Which problem is this PR solving?
Resolves #2225
Description of the changes
A new try catch block was inserted, for files that weren't parsed by the usual JSON.parse, would be parsed by the catch block assuming the objects are seperated by newline (which is mentioned in opentelemetry)
How was this change tested?
Test cases, and manual testing
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test