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

otelmux: Treat incoming trace ID as links rather than parents #3661

Merged
merged 16 commits into from
Apr 29, 2023

Conversation

elaous
Copy link
Contributor

@elaous elaous commented Mar 30, 2023

When a trace is sent with a trace context creates a problem where the traces is trying to making our traces a child of a parent that is not sent to the collectors. The otelmux package that we use doesn't provide a solution to this problem but otelhttp does. Using otelhttp as a guide marking endpoints as public is a good way to avoid orphan spans. This will treat incoming trace ID as links rather than parents.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 30, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: elaous / name: Samir El Aouar (bf091d5)

@elaous elaous marked this pull request as ready for review April 4, 2023 17:13
@elaous elaous requested a review from a team as a code owner April 4, 2023 17:13
Aneurysm9
Aneurysm9 previously approved these changes Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #3661 (fc0ea15) into main (c347e83) will increase coverage by 0.0%.
The diff coverage is 55.3%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #3661    +/-   ##
======================================
  Coverage   70.3%   70.4%            
======================================
  Files        147     149     +2     
  Lines       6959    7123   +164     
======================================
+ Hits        4899    5017   +118     
- Misses      1939    1974    +35     
- Partials     121     132    +11     
Impacted Files Coverage Δ
detectors/aws/ec2/version.go 0.0% <0.0%> (ø)
detectors/aws/ecs/version.go 0.0% <0.0%> (ø)
detectors/aws/eks/version.go 0.0% <0.0%> (ø)
detectors/gcp/version.go 0.0% <0.0%> (ø)
...thub.com/Shopify/sarama/otelsarama/test/version.go 0.0% <0.0%> (ø)
...github.com/astaxie/beego/otelbeego/test/version.go 0.0% <0.0%> (ø)
...tion/github.com/astaxie/beego/otelbeego/version.go 0.0% <0.0%> (ø)
...thub.com/aws/aws-sdk-go-v2/otelaws/test/version.go 0.0% <0.0%> (ø)
...om/emicklei/go-restful/otelrestful/test/version.go 0.0% <0.0%> (ø)
...n/github.com/gin-gonic/gin/otelgin/test/version.go 0.0% <0.0%> (ø)
... and 34 more

... and 3 files with indirect coverage changes

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@elaous elaous requested a review from Aneurysm9 April 5, 2023 17:24
@dmathieu dmathieu changed the title Treat incoming trace ID as links rather than parents otelmux: Treat incoming trace ID as links rather than parents Apr 14, 2023
dmathieu
dmathieu previously approved these changes Apr 14, 2023
@elaous elaous requested a review from dmathieu April 14, 2023 16:14
@elaous
Copy link
Contributor Author

elaous commented Apr 17, 2023

Hi, just wondering, what are the next steps. There is anything on my side that needs to be done?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

First, it looks like one of your commits is having CLA issues. Can you fix that?

@elaous
Copy link
Contributor Author

elaous commented Apr 25, 2023

First, it looks like one of your commits is having CLA issues. Can you fix that?
I updated with changes from master and it fixed the issue.

@pellared
Copy link
Member

@open-telemetry/go-maintainers This PR has already 3 approvals so it is probably good to merge.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 28, 2023

@open-telemetry/go-maintainers This PR has already 3 approvals so it is probably good to merge.

Reviews were re-requested for @Aneurysm9 and @dmathieu and they have not provided them.

@elaous
Copy link
Contributor Author

elaous commented Apr 28, 2023

@open-telemetry/go-maintainers This PR has already 3 approvals so it is probably good to merge.

Reviews were re-requested for @Aneurysm9 and @dmathieu and they have not provided them.

Sorry for the clueless question: How I dismissing a pull request review? Has been some time since I used Github.

@MrAlias
Copy link
Contributor

MrAlias commented Apr 28, 2023

@open-telemetry/go-maintainers This PR has already 3 approvals so it is probably good to merge.

Reviews were re-requested for @Aneurysm9 and @dmathieu and they have not provided them.

Sorry for the clueless question: How I dismissing a pull request review? Has been some time since I used Github.

NP, I don't know if you can based on the project settings and the default permissions.

I can dismiss them if you like?

@elaous
Copy link
Contributor Author

elaous commented Apr 28, 2023

@open-telemetry/go-maintainers This PR has already 3 approvals so it is probably good to merge.

Reviews were re-requested for @Aneurysm9 and @dmathieu and they have not provided them.

Sorry for the clueless question: How I dismissing a pull request review? Has been some time since I used Github.

NP, I don't know if you can based on the project settings and the default permissions.

I can dismiss them if you like?

Yes please.

@MrAlias MrAlias dismissed stale reviews from dmathieu and Aneurysm9 April 28, 2023 21:42

stale

@elaous
Copy link
Contributor Author

elaous commented Apr 29, 2023

There is anything I can do about codeconv/patch?

@MrAlias
Copy link
Contributor

MrAlias commented Apr 29, 2023

There is anything I can do about codeconv/patch?

If you would like to submit a follow up PR to help improve the projects overall coverage we would be happy to have it. But, for this PR, the coverage is good enough.

@MrAlias MrAlias merged commit e1ca274 into open-telemetry:main Apr 29, 2023
21 of 22 checks passed
@ash2k
Copy link
Contributor

ash2k commented May 3, 2023

Would be great to have this for the gRPC instrumentation too.

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