Propagation changes in anticipation of W3C support#12
Conversation
- DD_TRACE_PROPAGATION_STYLE - DD_TRACE_PROPAGATION_STYLE_INJECT - DD_TRACE_PROPAGATION_STYLE_EXTRACT
|
|
||
| // Parse the propagation styles from the configuration and/or from the | ||
| // environment. | ||
| // Exceptions make this section simpler. |
There was a problem hiding this comment.
How would it look with error-handling instead of exceptions?
There was a problem hiding this comment.
It would be a lot longer.
| // environment. | ||
| // Exceptions make this section simpler. | ||
| try { | ||
| const auto global_styles = styles_from_env(env::DD_TRACE_PROPAGATION_STYLE); |
There was a problem hiding this comment.
By this point, I'm wondering the value here of the macro aligning variable names with environment variable names.
There was a problem hiding this comment.
The benefit is that there's one way to add an environment variable: by adding an enum value. The list of environment variables consumed by nginx is then automatically up-to-date.
| if (!result.extraction_styles.datadog && !result.extraction_styles.b3 && | ||
| !result.extraction_styles.none) { | ||
| return Error{Error::MISSING_SPAN_EXTRACTION_STYLE, | ||
| "At least one extraction style must be specified."}; |
There was a problem hiding this comment.
Could we just fall back to a default in this instance, ie: datadog extraction / injection.
There was a problem hiding this comment.
True. The thing is, the only way to get this error is if the user explicitly said "the list of styles is empty."
If empty is a suitable default, then why have an explicit "none" style, especially if "none" only makes sense by itself? Better to just have "no styles" mean "no propagation."
Alas, "none" is in our spec.
Since the easiest way to get the default behavior is to not configure the styles, I'd rather have this error than add another way to get the default behavior.
|
Thanks for the review. I'll continue adding to this PR today. |
I'm reading through our internal RFC about adding support for W3C-style trace context propagation.
This pull request has changes in anticipation of that feature.