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

Enable displaying trace and span id on logs #4823

Merged
merged 10 commits into from
Mar 25, 2024

Conversation

BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Mar 19, 2024

To enable correlation between trace and logs trace_id and span_id should be included on the log messages.

Json:

{"timestamp":"2024-03-19T15:37:41.516453239Z","level":"INFO","trace_id":"54ac7e5f0e8ab90ae67b822e95ffcbb8","span_id":"9b3f88c602de0ceb","message":"Supergraph GraphQL response".....

Text:

2024-03-19T15:14:46.040435Z INFO trace_id: bbafc3f048b6137375dd78c10df18f50 span_id: 40ede28c5df1b5cc router{

Configuration options have been added in logging for text and json formats defaulting to true for json and false for text.

Json:

   exporters:
     logging:
       stdout:
         format:
           json:
             display_span_id: true
             display_trace_id: true

Text:

  exporters:
    logging:
      stdout:
        format:
          text:
            display_span_id: true
            display_trace_id: true

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

@BrynCooke BrynCooke requested a review from a team as a code owner March 19, 2024 16:00

This comment has been minimized.

@router-perf
Copy link

router-perf bot commented Mar 19, 2024

CI performance tests

  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • large-request - Stress test with a 1 MB request payload
  • const - Basic stress test that runs with a constant number of users
  • no-graphos - Basic stress test, no GraphOS.
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • xlarge-request - Stress test with 10 MB request payload
  • step - Basic stress test that steps up the number of users over time

@BrynCooke BrynCooke force-pushed the bryn/otel-trace-attributes-logging branch from 86e9020 to 6e84c39 Compare March 19, 2024 16:04
bryn added 2 commits March 20, 2024 08:55
To enable correlation between trace and logs trace_id and span_id should be included on the log messages.

 Json:
 ```
 {"timestamp":"2024-03-19T15:37:41.516453239Z","level":"INFO","trace_id":"54ac7e5f0e8ab90ae67b822e95ffcbb8","span_id":"9b3f88c602de0ceb","message":"Supergraph GraphQL response".....
 ```

 Text:
 ```
 2024-03-19T15:14:46.040435Z INFO trace_id: bbafc3f048b6137375dd78c10df18f50 span_id: 40ede28c5df1b5cc router{
 ```

 Configuration options have been added in logging for text and json formats defaulting to true.

Json:
```
   exporters:
     logging:
       stdout:
         format:
           json:
             display_span_id: true
             display_trace_id: true
```

Text:
```
  exporters:
    logging:
      stdout:
        format:
          text:
            display_span_id: false
            display_trace_id: false
```
@BrynCooke BrynCooke force-pushed the bryn/otel-trace-attributes-logging branch from c6afe84 to f70366f Compare March 20, 2024 08:55
Copy link
Contributor

@Geal Geal left a comment

Choose a reason for hiding this comment

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

this looks fine but do we want to activate by default 2 new high cardinality fields in the logs? Wouldn't that surprise users?

@BrynCooke
Copy link
Contributor Author

| this looks fine but do we want to activate by default 2 new high cardinality fields in the logs? Wouldn't that surprise users?

I see your point, but for instance New Relic and Datadog expect trace attributes to be on log messages, and the otel spec details that they should be there https://opentelemetry.io/docs/specs/otel/compatibility/logging_trace_context/#json-formats.

I think this'll give a better experience for the majority of users, and for logs carnality doesn't really matter.

@BrynCooke BrynCooke merged commit 38ca966 into dev Mar 25, 2024
14 checks passed
@BrynCooke BrynCooke deleted the bryn/otel-trace-attributes-logging branch March 25, 2024 10:14
@BrynCooke BrynCooke mentioned this pull request Mar 29, 2024
@maschwenk
Copy link

@BrynCooke trying to understand how this PR interacts with this one:

@Samjin
Copy link

Samjin commented Apr 28, 2024

@BrynCooke How to work with existing trace_id with value that our service stacks already generated from upstream? Is there a way to reassign the existing value to otel generated trace_id so we can avoid multiple trace_ids for all logging/trace/metrics?

eg. I was trying to override trace_id in span with our own value, but it's expecting a boolean only.
image

@bnjjj
Copy link
Contributor

bnjjj commented Apr 29, 2024

@Samjin I think you should use trace propagation for this use case https://www.apollographql.com/docs/router/configuration/telemetry/exporters/tracing/overview#propagation

@Samjin
Copy link

Samjin commented Apr 30, 2024

@Samjin I think you should use trace propagation for this use case https://www.apollographql.com/docs/router/configuration/telemetry/exporters/tracing/overview#propagation

Just tried it out with request header of trace-id, but it doesn't update the default OTEL trace_id value in span attribute and logs. Instead it did the opposite. It updated original request trace-id header and past to downstream.

@Samjin
Copy link

Samjin commented May 5, 2024

@Samjin I think you should use trace propagation for this use case https://www.apollographql.com/docs/router/configuration/telemetry/exporters/tracing/overview#propagation

Create a ticket to override default trace_id. #5056

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.

None yet

6 participants