Skip to content

fix(telemetry): improve error logging for custom trace_id generation #8149#8149

Closed
juancarlosjr97 wants to merge 12 commits intoapollographql:devfrom
juancarlosjr97:dev
Closed

fix(telemetry): improve error logging for custom trace_id generation #8149#8149
juancarlosjr97 wants to merge 12 commits intoapollographql:devfrom
juancarlosjr97:dev

Conversation

@juancarlosjr97
Copy link
Contributor

@juancarlosjr97 juancarlosjr97 commented Aug 26, 2025

This pull request improves logging in the CustomTraceIdPropagator implementation by enhancing the error message with additional context about the trace_id and the error.

First MR #7910 closed by GitHub

Logging enhancement:


Checklist

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

  • PR description explains the motivation for the change and relevant context for reviewing
  • PR description links appropriate GitHub/Jira tickets (creating when necessary)
  • Changeset is included for user-facing changes
  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Metrics and logs are added3 and documented
  • Tests added and passing4
    • Unit tests
    • Integration tests
    • Manual tests, as necessary

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. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices.

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

@juancarlosjr97 juancarlosjr97 requested a review from a team August 26, 2025 19:34
@juancarlosjr97 juancarlosjr97 requested a review from a team as a code owner August 26, 2025 19:34
@juancarlosjr97
Copy link
Contributor Author

Thanks for the update @bnjjj. I realised it is wrong PR number, but I have updated it :)

@juancarlosjr97 juancarlosjr97 requested a review from bnjjj September 2, 2025 09:58
@juancarlosjr97 juancarlosjr97 changed the title fix(telemetry): improve error logging for custom trace_id generation #7910 fix(telemetry): improve error logging for custom trace_id generation #8149 Sep 2, 2025
- Jaeger
- Zipkin

The router follows the [W3C Trace Context specification](https://www.w3.org/TR/trace-context/) for `trace_id` generation and propagation. OpenTelemetry uses 128-bit (32-character hexadecimal) trace IDs as defined in the W3C standard. When working with systems that do not follow this standard, the router provides format conversion options to ensure compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

What does "format conversion options" refer to here? i'm not really familiar with this aspect of router telemetry but I don't understand what this means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to this section - https://www.apollographql.com/docs/graphos/routing/observability/telemetry/trace-exporters/overview#request-configuration-reference.

The addition to the documentation is to make sure it is clear what the expectation for the trace ID

Copy link
Member

Choose a reason for hiding this comment

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

Ahh perfect thank you!

@abernix
Copy link
Member

abernix commented Sep 10, 2025

@mergify copy dev

@juancarlosjr97 Thanks so much for opening this! Now that it looks like approval is on the horizon, we're going to move this PR over to a direct branch on the repository so the full CI run can happen, including having access to our GITHUB_TOKEN which allows us to go over the GitHub anonymous download rate-limits which aren't currently being permitted on your PR.

You will briefly see a new PR show up in the metadata here, and it will preserve your contribution credit!

@mergify
Copy link
Contributor

mergify bot commented Sep 10, 2025

copy dev

✅ Pull request copies have been created

Details

bnjjj pushed a commit that referenced this pull request Sep 25, 2025
…(copy #8149) (#8246)

Co-authored-by: Juan Carlos Blanco Delgado <juancarlosjr97@gmail.com>
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Co-authored-by: Renée Kooi <renee.kooi@apollographql.com>
@juancarlosjr97
Copy link
Contributor Author

Are we happy to close this MR as the #8246 has been merged @bnjjj / @abernix?

@abernix
Copy link
Member

abernix commented Sep 25, 2025

Yup! thank you so much for the contribution! Closing in favor of the now merged #8246.

@abernix abernix closed this Sep 25, 2025
@abernix abernix mentioned this pull request Oct 27, 2025
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.

4 participants