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 trace.id and transaction.id #519

Merged
merged 4 commits into from
Aug 19, 2019

Conversation

felixbarny
Copy link
Member

Closes #99

Currently, apm is nested under use-cases which means those fields are not part of the default ecs index template. Are there plans to change that? It would make sense to have the trace.id and transaction.id in the template. You know, for log correlation.

@felixbarny felixbarny requested review from roncohen, webmat and axw August 13, 2019 17:39
@webmat
Copy link
Contributor

webmat commented Aug 14, 2019

I like the idea of adding support for tracing in ECS.

However we try to limit the amount of top level field sets. For example, each field set is represented in the sidebar in the documentation.

So adding two distinct field sets that each contain one id field only, and are closely related seems a bit cumbersome. Could both concepts be nested under a common field set, perhaps tracing.*? Or are they distinct enough that this wouldn't make sense?

Also, is there possibly other fields that could be related to this matter, that may be added in the future? We don't need to add them now, but just knowing that there may be something more eventually can be helpful in designing this.

@ruflin
Copy link
Contributor

ruflin commented Aug 15, 2019

++ on adding support for tracing / transactions to ECS. Could the above be adopted by APM (apm-server)? @graphaelli @simitt would be great to get your input on this one.

@webmat
Copy link
Contributor

webmat commented Aug 15, 2019

My understanding is that trace.id and transaction.id are already the fields used by APM.

@graphaelli
Copy link
Member

That's right, they are already in use, including for linking between observability UIs.

@webmat
Copy link
Contributor

webmat commented Aug 15, 2019

I didn't realize you had modified the use case files. These have fallen out of date and aren't part of the spec at all. The intent for these was to showcase how users can start from ECS and add their custom fields around ECS.

If we want to add tracing support in ECS per se, we'll need to add these fields to new YAML files in schemas. Namely schemas/trace.yml and schemas/transaction.yml. I'll have to check if we can instead reduce the noise and have both these fields inside the same file, and inside the same documentation section (e.g. have a "tracing" section containing both trace.id and transaction.id).

Question before we work on adding this into ECS, though. Is there a desire for users to ingest tracing events from other products into Elasticsearch? If so, could you provide examples of other such sources?

@felixbarny
Copy link
Member Author

Is there a desire for users to ingest tracing events from other products into Elasticsearch? If so, could you provide examples of other such sources?

Our agents support enhancing logs with those identifiers. That supports linking from the Infra Logs UI to the APM UI and vice-versa. This is a GA feature and we already rely on those identifiers.

Other than that, the canonical way of ingesting trace data into Elasticsearch would be to use the APM server, which has it's own definition (a JSON schema) of the data format.

We should probably only include trace.id and transaction.id to ECS for now.

I'll have to check if we can instead reduce the noise and have both these fields inside the same file, and inside the same documentation section (e.g. have a "tracing" section containing both trace.id and transaction.id).

That would be great!

@MikePaquette
Copy link
Contributor

@webmat I agree with your point about "one-field" field sets being kind of cumbersome, and we do already have a precedent for including fields with different prefixes in the same YAML file with the base fields, but I'd be +1 for creating separate YAML files for trace.* and transaction.*, and probably adding .name fields to each in addition to the .id fields.

@felixbarny
Copy link
Member Author

Actually, there is no trace.name. There's also no explicit entity or document describing a trace. A trace is the combination of all transaction documents with the same trace.id.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Ok let's get this in simply for now.

I will work to merge them into one "tracing" section in a separate PR, after this is in.

So to get this in, we should:

  • Remove the changes in the use case files
  • create schemas/trace.yml for trace.id
  • create schemas/transaction.yml for transaction.id

And here's more of a general discussion point. Should we discuss how people can use these tracing identifiers in non-APM situations? E.g. If someone identifies traces based on web transactions like APM, but uses Cloudflare's Ray ID as the identifier; would that be ok?

@webmat
Copy link
Contributor

webmat commented Aug 16, 2019

I agree with sticking to only both IDs for now. And I think the current descriptions are good starting points

@webmat
Copy link
Contributor

webmat commented Aug 16, 2019

Looks like some of the files need a final run of make & commit the changes ;-)

@webmat
Copy link
Contributor

webmat commented Aug 16, 2019

Small typo in "A trace groups multipe" => "multiple" :-)

@felixbarny
Copy link
Member Author

🤦‍♂ good catch, thanks

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution and the adjustments :-)

@felixbarny felixbarny merged commit a2f70a7 into elastic:master Aug 19, 2019
@felixbarny felixbarny deleted the trace-id-transaction-id branch August 19, 2019 07:42
@ruflin
Copy link
Contributor

ruflin commented Aug 19, 2019

Thanks for pushing this through. Looks like APM has landed in ECS ;-) @graphaelli For awareness.

@webmat
Copy link
Contributor

webmat commented Aug 19, 2019

And just to make sure everybody knows, this is already on the website in the "master" version of ECS, much like our other products.

https://www.elastic.co/guide/en/ecs/master/ecs-tracing.html

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.

APM/Log correlation: add span ID and trace ID
6 participants