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

feat: add traceID and pass it to ES #489

Merged
merged 1 commit into from
Aug 1, 2024
Merged

feat: add traceID and pass it to ES #489

merged 1 commit into from
Aug 1, 2024

Conversation

SimonThordal
Copy link
Contributor

@SimonThordal SimonThordal commented Jul 30, 2024

Adds trace IDs to requests as based on https://www.w3.org/TR/trace-context/#processing-model and passes them to ES. The major win here is that we're not breaking traces if we're part of a clients distributed stack.

We're not fully compliant as we're not doing full validation of the incoming traces or inspecting the flags, but we currently correctly participate in or initiate traces and add the trace context to our logs. ES also emits the trace context in logs, but it does not seem like OS supports it quite yet.

I removed our trace ID implementation completely, but we could also deprecate it.

@SimonThordal SimonThordal force-pushed the add-traceid branch 6 times, most recently from 2e0af2a to 6c28a82 Compare July 31, 2024 08:08
@pudo
Copy link
Member

pudo commented Jul 31, 2024

Wow, that's quite a chunk of code. Would it make sense to put in a few unit tests for the tracer classes? They're the type of thing that is going to remain untouched for years after we merge this until someone points out that it hasn't worked since three releases....

@SimonThordal
Copy link
Contributor Author

Wow, that's quite a chunk of code. Would it make sense to put in a few unit tests for the tracer classes? They're the type of thing that is going to remain untouched for years after we merge this until someone points out that it hasn't worked since three releases....

Sure, I have a Postman test suite that I'm using for it, but we can add them as integration tests as well.

@SimonThordal
Copy link
Contributor Author

Wow, that's quite a chunk of code. Would it make sense to put in a few unit tests for the tracer classes? They're the type of thing that is going to remain untouched for years after we merge this until someone points out that it hasn't worked since three releases....

Now with a test.

Copy link
Member

@pudo pudo left a comment

Choose a reason for hiding this comment

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

Nice work!

@SimonThordal SimonThordal merged commit 6585c82 into main Aug 1, 2024
6 checks passed
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.

2 participants