Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

feat: trace context traceparent header support#74

Merged
lidel merged 1 commit intomainfrom
issue/68
Apr 11, 2023
Merged

feat: trace context traceparent header support#74
lidel merged 1 commit intomainfrom
issue/68

Conversation

@hacdias
Copy link
Collaborator

@hacdias hacdias commented Apr 4, 2023 •
edited by lidel
Loading

hacdias
hacdias commented Apr 4, 2023
View reviewed changes
@hacdias hacdias self-assigned this Apr 4, 2023
@hacdias hacdias marked this pull request as ready for review April 4, 2023 11:16
@hacdias hacdias requested a review from lidel April 4, 2023 11:16
@hacdias hacdias force-pushed the issue/68 branch 2 times, most recently from a53e879 to 0c859c1 Compare April 5, 2023 09:48
@hacdias hacdias changed the title feat: add traceparent header feat: support Trace Context Apr 5, 2023
lidel
lidel reviewed Apr 6, 2023
View reviewed changes
Copy link
Collaborator

@lidel lidel left a comment •
edited by hacdias
Loading

Choose a reason for hiding this comment

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

Thank you @hacdias, left a comment above.

I think the next steps here is to make this universal feature:

  • support what Kubo supports (docs)
  • confirm that traceparent is applied to all requests sent by bifrost-gateway to other systems
    • KUBO_RPC_URL → routing.go
    • when PROXY_GATEWAY_URL is set → blockstore_proxy.go
    • when STRN_ORCHESTRATOR_URL is set → open PR in caboose that ensures header is forwarded to Saturn L1 with every request?
      - This should work as per #74 (comment)
  • add "How to use tracing?" to FAQ in README.md
    • example how to pass traceparent
    • example how to trace with Jaeger UI (link to /docs/environment-variables.md#tracing based on Kubo?)

@hacdias
Copy link
Collaborator Author

hacdias commented Apr 6, 2023 •
edited
Loading

I'm using Kubo @ ipfs/kubo#9801 for testing.

Connected traces of Bifrost Gateway proxying requests to Kubo (PROXY_GATEWAY_URL):

image

Connected traces of Bifrost Gateway proxying IPNS records to Kubo (KUBO_RPC_URL):

image

STRN_ORCHESTRATOR_URL should work since we wrap the caboose transport with otelhttp and caboose already uses the request with context:

https://github.com/filecoin-saturn/caboose/blob/34e6ab0f0e60ed6bef02b06242573aa5ab06bd37/fetcher.go#L215-L219

@hacdias hacdias requested a review from lidel April 6, 2023 14:27
@hacdias hacdias changed the title feat: support Trace Context feat: support trace context tracing with OTel Apr 6, 2023
@lidel lidel mentioned this pull request Apr 6, 2023
Closed
@hacdias hacdias force-pushed the issue/68 branch 2 times, most recently from 5b1290f to 3e682ed Compare April 11, 2023 09:45
@hacdias hacdias mentioned this pull request Apr 11, 2023
Merged
lidel
lidel approved these changes Apr 11, 2023
View reviewed changes
Copy link
Collaborator

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you @hacdias!

Demo

OTEL_TRACES_EXPORTER=jaeger with Jaeger UI:

Screenshot 2023-04-12 at 01-02-57 Jaeger UI

@lidel lidel changed the title feat: support trace context tracing with OTel feat: trace context traceparent header support Apr 11, 2023
@lidel lidel merged commit 660ec70 into main Apr 11, 2023
@lidel lidel deleted the issue/68 branch April 11, 2023 23:48
guanzo pushed a commit to guanzo/bifrost-gateway that referenced this pull request May 12, 2023
@hacdias @guanzo
feat: trace context traceparent header support (ipfs-inactive#74)
128005e 
Context:
https://www.w3.org/TR/trace-context/
ipfs-inactive#68
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@lidel lidel lidel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hacdias hacdias

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support Trace Context HTTP headers

2 participants

@hacdias @lidel