Span service target fields specification#627
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
|
basepi
left a comment
There was a problem hiding this comment.
Just one comment about db.instance. Otherwise I think this is looking good!
|
This specification is now ready for the next step, I've just renamed it to reflect that. If we follow "The Process" ® the next step is getting one agent approval, @basepi since you already looked at it, would you mind doing that 🙏 ? |
Co-authored-by: Felix Barnsteiner <felixbarny@users.noreply.github.com>
| - `span.context.service.target.type` : [ECS service.type](https://www.elastic.co/guide/en/ecs/current/ecs-service.html#field-service-type) | ||
| , optional. | ||
| - `span.context.service.target.name` : [ECS service name](https://www.elastic.co/guide/en/ecs/current/ecs-service.html#field-service-name) | ||
| , optional, ignored if `span.context.service.target.type` is not provided. |
There was a problem hiding this comment.
From the APM server's perspective, does this mean that for validating span.context.service.target it should expect span.context.service.target.type to be a required field?
There was a problem hiding this comment.
Just to be sure, here do you mean 1) in the data sent by agents, 2) to validate data stored to ES or 3) both ? I'll assume 1) here in my answer.
The whole span.context.service.target is optional, but I did not thought that we could maybe enforce a mandatory property within it, so maybe having span.context.service.target.type mandatory (but potentially empty) could be a good option, would that make things simpler on the server side for ingestion ?
However, one good reason to keep most of them optional is that the mandatory fields tends to stay forever for backwards compatibility after they are added, so while we don't have currently any use-case for that yet, it could help us later.
Also, another reason to stay cautious with mandatory fields is that if there is any extra complexity on the apm-server side, we can always fix it through code but the protocol can't be modified as easily. Another example of similar strategy in another context is with protobuf in version >=3 where all the attributes are optional for similar reasons.
Maybe @felixbarny could enlighten us with the agent protocol history and what should be the best strategy here.
There was a problem hiding this comment.
After discussing with @SylvainJuge we have a decision to have APM-server expect span.context.service.target with at-least one field, name or type, as non-empty.
Implementation checklist
This PR is considered a work-in progress draft until the following items have been addressed:
destination.service.resource.UPDATE: we need to keep the
resourcefor older servers + compatibility with UIAdd Jaeger compatibilitySkipped for now, maybe in a follow-up PR.tracing-instrumentation-db.mdtracing-spans-compress.mdtracing-spans-dropped-stats.mdUPDATE: only infer on
resourcefield, back-fill new fields on a best-effort.This is a new spec or a bigger enhancement
YesAdd a section to the spec how agents should apply sanitization (such assanitize_field_names)NoWhy:CODEOWNERS)Fixes #622
Summary of the impact on product: