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

experimental_response_trace_id - provide ability for returned value to be base 10 representation of number #4422

Closed
theJC opened this issue Dec 21, 2023 · 3 comments · Fixed by #4530
Assignees
Labels

Comments

@theJC
Copy link
Contributor

theJC commented Dec 21, 2023

Describe the solution you'd like

Currently when using experimental_response_trace_id, the response header is a base 16 number
ie:
trace-id: 000000000000000000023096b9f0c199

At Indeed we use Datadog for our APM tracing SaaS, and it deals with trace-id's in base 10 in its UI, so when pulling up a trace, the user needs to convert the trace-id from base16 to base 10 and then use that number to pull the trace up in the UI.

We would like the capability for this feature support the ability for a base 10 number to be the value returned in the header, in this case that would be:
trace-id: 616373876212121

@bnjjj
Copy link
Contributor

bnjjj commented Jan 8, 2024

A solution could be to add a new format configuration in experimental_response_trace_id that would support both hexadecimal and decimal (default set to hexadecimal). And we would have to check that configuration here and use t.to_u128().to_string() instead of t.to_string() for decimal format.

What do you think about this configuration @BrynCooke ?

If you have time to open a PR @theJC I would be happy to help :)

@bnjjj bnjjj added the good first issue Good for newcomers label Jan 8, 2024
nicholascioli added a commit that referenced this issue Jan 22, 2024
resolves #4422

Previously, when the `experimental_response_trace_id` was enabled,
the router would append the trace id, if present, to the headers
of the response in hexadecimal format, left-padded with 0s. This
commit allows specifying through the new config option `format`
whether to have that value be hexadecimal (the previous behaviour
and the new default) or decimal.

Below is an example of the new configuration option in action:

```yaml
telemetry:
  apollo:
    client_name_header: name_header
    client_version_header: version_header
  exporters:
    tracing:
      experimental_response_trace_id:
        enabled: true
        header_name: trace_id
        format: decimal # Optional, defaults to hexadecimal
```
nicholascioli added a commit that referenced this issue Jan 22, 2024
resolves #4422

Previously, when the `experimental_response_trace_id` was enabled,
the router would append the trace id, if present, to the headers
of the response in hexadecimal format, left-padded with 0s. This
commit allows specifying through the new config option `format`
whether to have that value be hexadecimal (the previous behaviour
and the new default) or decimal.

Below is an example of the new configuration option in action:

```yaml
telemetry:
  apollo:
    client_name_header: name_header
    client_version_header: version_header
  exporters:
    tracing:
      experimental_response_trace_id:
        enabled: true
        header_name: trace_id
        format: decimal # Optional, defaults to hexadecimal
```
@theJC
Copy link
Contributor Author

theJC commented Jan 22, 2024

Beat me to the punch! 😉 Thanks @nicholascioli !

nicholascioli added a commit that referenced this issue Jan 23, 2024
resolves #4422

Previously, when the `experimental_response_trace_id` was enabled, the
router would append the trace id, if present, to the headers of the
response in hexadecimal format, left-padded with 0s. This commit allows
specifying through the new config option `format` whether to have that
value be hexadecimal (the previous behaviour and the new default) or
decimal.

Below is an example of the new configuration option in action:

```yaml
telemetry:
  apollo:
    client_name_header: name_header
    client_version_header: version_header
  exporters:
    tracing:
      experimental_response_trace_id:
        enabled: true
        header_name: trace_id
        format: decimal # Optional, defaults to hexadecimal
```

Note: Writing tests for this would be useful, but it seems very
difficult to prime a trace ID that isn't randomly generated for unit
testing.

<!-- start metadata -->
---

**Checklist**

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

- [x] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

**Notes**

[^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.
@kindermax
Copy link
Contributor

kindermax commented Mar 15, 2024

Hi, is it possible to add a uuid4 format as well and also using it in CustomTraceIdPropagator here apollo-router/src/plugins/telemetry/mod.rs?
p.s. Sorry for touching an old/closed issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants