-
Notifications
You must be signed in to change notification settings - Fork 7
Support for OpenTracing 0.33.0 #29
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
Conversation
|
Hi @agarbutt , Thanks for the contribution! I'm going to take a look at this. |
jeffalder
left a comment
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.
1 - please ensure the license header is present on all added files
2 - check the use of AtomicBoolean
looks good otherwise.
| .optionallyWithTag( | ||
| "aws.lambda.eventSource.arn", EventSourceParser.parseEventSourceArn(input)) | ||
| .optionallyWithTag( | ||
| "aws.lambda.coldStart", TracingRequestHandler.isColdStart.getAndSet(false)) |
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.
This is the only isColdStart referenced any longer. Should it simply be an static AtomicBoolean on this class?
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.
Great catch! This is a bad refactoring. See my updated commit.
|
We should also update the AWS Lambda Tracer to support OpenTracing 0.33 in conjunction with this PR for the SDK. https://newrelic.atlassian.net/browse/JAVA-5898 |
- Corrected a bad refactoring referencing `isColdStart` - Added Copyright headers.
|
Thanks for the feedback @jeffalder. I believe I have addressed the concerns you raised. I'd be happy to help contribute, if that would be helpful, @jasonjkeller |
|
We'll have to ship an update to the other side of this to match up OT versions. |
Support for #18