add traceparent header support #81
Conversation
There was a problem hiding this comment.
Nice! Code looks fine, just some minor RFC-adherence nits and questions about usage.
FWIW, I think there's a reasonable case to be made for omitting these types of internals (managed_by_id too) from the provider config and only accepting inputs as environment variables. That essentially makes them "secret" (as in undocumented, not sensitive) configuration inputs. I can't imagine users wanting to set this from their own environments. And so it just creates clutter in the schema/docs.
observe/provider.go
Outdated
| @@ -110,6 +110,12 @@ func Provider() *schema.Provider { | |||
| Optional: true, | |||
| Description: "ID of an Observe object that serves as the parent (managing) object for all resources created by the provider (internal use).", | |||
| }, | |||
| "traceparent": { | |||
| Type: schema.TypeString, | |||
| DefaultFunc: schema.EnvDefaultFunc("W3C_TRACEPARENT", nil), | |||
There was a problem hiding this comment.
| DefaultFunc: schema.EnvDefaultFunc("W3C_TRACEPARENT", nil), | |
| DefaultFunc: schema.EnvDefaultFunc("TRACEPARENT", nil), |
I'm unable to find any cases where this is used as a configuration input (versus just internally to the code):
https://github.com/search?q=%22W3C_TRACEPARENT%22&type=code&p=1
I did find this:
observe/provider.go
Outdated
| Type: schema.TypeString, | ||
| DefaultFunc: schema.EnvDefaultFunc("W3C_TRACEPARENT", nil), | ||
| Optional: true, | ||
| Description: "Optional traceparent identifie.", |
There was a problem hiding this comment.
| Description: "Optional traceparent identifie.", | |
| Description: "A trace identifier to attach to all HTTP requests in the traceparent header (https://www.w3.org/TR/trace-context/#traceparent-header)", |
observe/provider.go
Outdated
| @@ -110,6 +110,12 @@ func Provider() *schema.Provider { | |||
| Optional: true, | |||
| Description: "ID of an Observe object that serves as the parent (managing) object for all resources created by the provider (internal use).", | |||
| }, | |||
| "traceparent": { | |||
There was a problem hiding this comment.
Wondering whether it's worth adding validation to this field, since traceparent is not just an opaque ID and instead has multiple well-structured parts:
https://www.w3.org/TR/trace-context/#traceparent-header-field-values
The RFC says multiple times that "vendors MUST ignore" the traceparent if any of the fields are invalid.
There was a problem hiding this comment.
Thought about this, and leaned against adding validation. Mainly as we are just passing the context through to the server. And the extract logic on the server does validate and ignore if the fields are invalid (essentially starts a new trace).
There was a problem hiding this comment.
If it's exposed to users I tend to think more validation is better if it can turn a silent failure into an explicit error. By not officially exposing the option to users that safety/predictability imperative largely goes away. So 👍🏻 to no validation in combo with the move to env only.
|
Oh and re provider config fields vs env only, if you do keep the explicit field, you'll need to |
|
Moved traceparent to just an envvar. Did not port the |
When leveraging the provider via code (tf-exec) it's useful to pass along the trace context.
This change adds support for setting the W3C traceparent header via an optional provider level property.