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 context fields to GcpLayout.json #2498

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

aabmass
Copy link
Contributor

@aabmass aabmass commented Apr 22, 2024

This PR adds a few tracing related JSON fields to the included GcpLayout.json event template which are documented in the Cloud Logging docs.

The intended source is well known OpenTelemetry MDC keys encoded as a hex strings following the W3C Trace Context standard.

  • MDC trace_id -> logging.googleapis.com/trace
  • MDC span_id -> logging.googleapis.com/spanId
  • true -> logging.googleapis.com/trace_sampled. This is unconditionally set to true because there is currently no way to derive it from W3C trace_flags with JTL (see Can JsonTemplateLayout output JSON boolean values? #2482). I'm OK to remove this if needed, as it will affect existing users even if the MDC keys are not set. However it's absence does break some GCP UI features.

@aabmass aabmass marked this pull request as ready for review April 22, 2024 16:06
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

@aabmass, thanks so much for the contribution! Much appreciated! 🙇

A quick question just before merging it: If trace_id and/or span_id MDC values are missing, logging.googleapis.com/trace_sampled will still be set to true without logging.googleapis.com/trace and/or logging.googleapis.com/spanId fields. Will this still be okay?

I would also really appreciate it if you can create a src/changelog/.2.x.x/add_GcpLayout_tracing_support.xml changelog entry file.

@vy vy self-assigned this Apr 22, 2024
@vy vy added enhancement Additions or updates to features layouts Affects one or more Layout plugins labels Apr 22, 2024
@aabmass
Copy link
Contributor Author

aabmass commented Apr 22, 2024

Thanks for the quick review!

A quick question just before merging it: If trace_id and/or span_id MDC values are missing, logging.googleapis.com/trace_sampled will still be set to true without logging.googleapis.com/trace and/or logging.googleapis.com/spanId fields. Will this still be okay?

It should be OK and doesn't have any effect in the Cloud Console if the IDs aren't present–the trace icon won't appear regardless without the IDs. Here is how the three fields correspond to the UI:
image

If you click on the trace icon (highlighted in the image, only visible if the IDs are present), it opens a dropdown and the trace_sampled flag controls whether this button is greyed out or not:

trace_sampled = true trace_sampled = false
image image

I think setting trace_sampled = true might cause some confusion (especially if querying by that field) but it keeps the "View trace details" from being greyed out

@aabmass
Copy link
Contributor Author

aabmass commented Apr 22, 2024

A quick question just before merging it: If trace_id and/or span_id MDC values are missing, logging.googleapis.com/trace_sampled will still be set to true without logging.googleapis.com/trace and/or logging.googleapis.com/spanId fields. Will this still be okay?

Would appreciate a quick LGTM from @dashpole, @punya, or @jsuereth on this point before merging

@aabmass
Copy link
Contributor Author

aabmass commented Apr 22, 2024

I would also really appreciate it if you can create a src/changelog/.2.x.x/add_GcpLayout_tracing_support.xml changelog entry file.

Added

@vy
Copy link
Member

vy commented Apr 23, 2024

@aabmass, thanks so much! I will merge this once CI passes and cherry-pick it to main too.

@vy vy merged commit e8a5053 into apache:2.x Apr 23, 2024
9 checks passed
vy added a commit that referenced this pull request Apr 23, 2024
Co-authored-by: Volkan Yazıcı <volkan@yazi.ci>
@aabmass aabmass deleted the gcplayout-trace-context branch April 23, 2024 17:43
@ppkarwasz ppkarwasz added this to the 2.24.0 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions or updates to features layouts Affects one or more Layout plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants