Skip to content
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

Add option to always create trace ID #243

Merged
merged 4 commits into from
Dec 1, 2020

Conversation

friscoMad
Copy link
Contributor

Issue #, if available:
Fixes #238

Description of changes:
This PR adds a new method in the builder and in the recorder to force NoOpSegments to have a valid individual Trace ID.
I am not entirely happy with the variable name so suggestions are accepted.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution! LGTM overall, just a naming change recommendation as requested.

* This could be useful for example in case the Trace ID is logged to be able to aggregate all logs from a
* single request
*/
public AWSXRayRecorderBuilder withAlwaysCreateTraceId() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think withForcedTraceIdGeneration would sound better.

* Configures this {@code AWSXRayRecorder} to add valid TraceId in all segments even NoOp ones that usually have
* a fixed value.
*/
public void alwaysCreateTraceID(final boolean alwaysCreateTraceId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be best if this method was explicitly named as a setter, e.g. setForcedTraceIdGeneration.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@willarmiros willarmiros merged commit be0b296 into aws:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add configuration option to create traceIds for NoOpSegments
2 participants