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

[Feature]: otelfiber - propagate tracing context as headers in outbound response #848

Merged
merged 3 commits into from
Feb 13, 2024
Merged

[Feature]: otelfiber - propagate tracing context as headers in outbound response #848

merged 3 commits into from
Feb 13, 2024

Conversation

ledex
Copy link
Contributor

@ledex ledex commented Nov 13, 2023

This fixed #836.

@ReneWerner87 ReneWerner87 added the ✏️ Feature New feature or request label Nov 13, 2023
@emanuelef
Copy link
Contributor

Hi, I wonder if we could add a test for that.

@ReneWerner87
Copy link
Member

Hi, I wonder if we could add a test for that.

@ledex is this possible? can you also update your branch with the latest master changes

@ledex
Copy link
Contributor Author

ledex commented Nov 17, 2023

Alright, the tests are there. I noticed that the outbound propagation only takes effect if otelfiber.WithPropagators is set. It does not work with the default TextMapPropagator. Is that okay for you or should I investigate this further?

@ledex
Copy link
Contributor Author

ledex commented Nov 24, 2023

@emanuelef Can you check the pull-request again?

Copy link
Contributor

@emanuelef emanuelef left a comment

Choose a reason for hiding this comment

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

LGTM but still wondering which common use case is addressing

@ReneWerner87
Copy link
Member

@ledex can you explain the use case ?

@ledex
Copy link
Contributor Author

ledex commented Nov 27, 2023

@emanuelef So my usecase is that I have one component (A) that first calls my service (B, implemented with Fiber and using the otelfiber middleware) and then another service (C).
When A calls B, A does not provide any tracing-context, but B creates a new tracing-context, if none is present. Since it would be useful that B and C write spans to the same trace, B needs to propagate the tracing-context back to the calling service (A), so that A can use it when calling C.

But I've read the B3-specs just now and I'm not sure if that's the correct way to do it... But we do it here at our company like this.

@emanuelef
Copy link
Contributor

As I mentioned in the other thread I was able to pass the trace id with the current otelfiber: https://medium.com/stackademic/start-using-opentelemetry-with-go-fiber-1005697d841f.
I'll try again with latest versions and update here if I see the same behaviour.

@emanuelef
Copy link
Contributor

I tried again calling one secondary component using otelfiber:
https://github.com/emanuelef/go-fiber-honeycomb/blob/main/secondary/main.go

Screenshot 2023-11-27 at 16 30 09

But I cannot comment on B3-specs, if there's the need to support that.
The use case you describe seems to be supported already.

@ReneWerner87
Copy link
Member

@ledex what is your opinion on this, he (@emanuelef ) thinks it is already possible

@ledex
Copy link
Contributor Author

ledex commented Dec 11, 2023

@emanuelef So I think we talk past each other. In your example the caller calls your service A, which then calls another service B. My usecase is the following:

sequenceDiagram
    autonumber

    participant Caller
    participant Service A
    participant Service B

    Caller ->>+ Service A: call service A
    Note right of Service A: create tracing context (`foo`)
    Note right of Service A: do something
    Note right of Service A: write spans
    Service A ->>+ Caller: return result and tracing context (`foo`)
    Caller ->>+ Service B: call service B with tracing context (`foo`)
    Note right of Service B: do something
    Note right of Service B: write spans
    Service B ->>+ Caller: return reault and tracing context (`foo`)
Loading

In call 2 I want to return the tracing context from Service A to the caller, so that it can use it to call Service B. Or would you suggest that the caller needs to create the tracing context and send it to both Service A and Service B?

@emanuelef
Copy link
Contributor

emanuelef commented Dec 15, 2023

@ledex I think I would probably just create the context in the caller and then it will pass around the trace parent.
From what I see in the graph it seems that in your use case you want the initiatiator to be Service A and not the caller.
Have you tried ?
Is there any reason why the caller shouldn't be present in the trace ?

PS: Nice chart and integration with GitHub

@ledex
Copy link
Contributor Author

ledex commented Dec 15, 2023

Yes that's right, service A should be the initiator in my use-case. We are providing this our service to other applications within our company, so we cannot be certain who calls our service and weather or not they provide a tracing context. In any case we want to return the tracing-context back to the caller, so that they can see it in their logs and can access the corresponding trace.

@emanuelef
Copy link
Contributor

So you want to pass back the context from Service A with the new trace and keep using that from that first call on.
But to me the caller is making two different requests, so shouldn't those be two different traces ?
In case you want to have all those spans in the same trace I wonder what the best practice using OpenTelemetry., because is like passing back the traceparent.
I'd need to do some research as this is a use case I've never needed.

@ReneWerner87
Copy link
Member

@emanuelef @ledex any outcome ? should i merge these changes?

@ledex
Copy link
Contributor Author

ledex commented Feb 13, 2024

@ReneWerner87 @emanuelef At the moment we have a work-around build for this in our code base, but I can see why you might not want this feature in otelfiber. I'm also not that familiar with the B3 specs and that's really up to you.

I just thought it might be helpful for others and it does not really break anything, however, it is quite a specific usecase.

@gaby
Copy link
Member

gaby commented Feb 13, 2024

If its not a breaking change, I'd say we merge it?

@ReneWerner87 ReneWerner87 merged commit d5a794c into gofiber:main Feb 13, 2024
52 checks passed
@ledex ledex deleted the feature/issue-836/propergate-tracing-headers-in-outbound-response branch February 13, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Feature New feature or request 💡 Help wanted Extra attention is needed ♻ Wait for Response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 [Feature]: otelfiber - Propergate tracing headers in outbound response
4 participants