-
Notifications
You must be signed in to change notification settings - Fork 16
Update span enrichment to clear the span ID and span Name #254
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
This allows the span id and name to be only represented by the elastic attributes (`span.id`, `span.name` or `transaction.id` , `transaction.name`) only
enrichments/config/config.go
Outdated
| // ClearSpanID sets the span ID to an empty value so that the | ||
| // ID is only represented by the `transaction.ID` attribute. | ||
| // Applicable only when ID is enabled. | ||
| // Disabled by default. | ||
| ClearSpanID AttributeConfig `mapstructure:"clear_span_id"` |
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.
Is this relevant to ElasticSpanConfig? The struct's doc comment says:
// ElasticSpanConfig configures the enrichment attributes for the spans
// which are NOT identified as elastic transaction.
... so it should always be span.*, and never transaction.*?
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.
Oh I messed up the config comments. The comments should mention span.id and span.name for the ElasticSpanConfig. And the ElasticTransactionConfig should mention transaction.id and transaction.name
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.
I see, makes sense now - thanks :)
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.
Actually... no, I'm still confused.
If we're enriching a span, and it should have a span.id, why would we need to clear span ID?
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.
Oh, I think I see now: we're adding an "id" attribute to the span attributes, and then clearing the top-level ID field. This seems a bit unnecessary. Why not just leave the top-level ID field set, and not add another attribute?
Could we remove "ID", "Name", "ClearSpanID", and "ClearSpanName" from ElasticSpanConfig?
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.
@axw @gregkalapos Yes I think that makes sense since the es exporter will continue to use the top-level ID and Name fields. And my original intent was to avoid incorrectly adding span.* fields for Elastic Transactions. I will test this e2e locally to make sure I am not missing anything and push a change to remove "ID", "Name", "ClearSpanID", and "ClearSpanName" from ElasticSpanConfig.
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.
I removed the above mention enrichments/configs from the ElasticSpanConfig. span.id and span.name continue to be added to the final document by the es exporter
gregkalapos
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.
Looks ok to me.
I think @axw raises a good point that for spans this may not be need, only for transactions.
I don't really have a strong opinion on that - this way it's more consistent (spans and transactions are treated the same), if we drop the config for spans, then we don't do this "unnecessary" step for spans, but spans and transactions are treated differently.
To me this seems ok as is now in the PR - but would be ok with it, if we'd drop span configs as well.
…e span.id and span.name enrichment since it is not required, the es exporter will add these fields
axw
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.
Thanks @isaacaflores2!
Overview
This PR updates the span enrichment to clear the span id and span name. This allows the span id and name to be only represented by the elastic attributes (
span.id,span.nameortransaction.id,transaction.name) only. The ES Exporter can then use both the elastic attributes and the SpanID and SpanName in the ECS encoder and the resulting document will contain the correct attributes based on the enrichment applied in theelasticapmprocessor/opentelemetry-lib.The
clear_span_idandclear_span_nameconfigs are disabled by default for backwards compatibility. Collectors that do not require strict parity with the apm-data flow can keep these configs disabled (example should be the EDOT collectors). Collectors that would like to closely match the apm-data behavior can enable these configs as needed.Related to: open-telemetry/opentelemetry-collector-contrib#45014 and elastic/opentelemetry-collector-components#943
Testing
processor/elasticapmconfig: