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

Add AddLink method to DummySpan #192

Merged
merged 3 commits into from
Apr 22, 2024
Merged

Add AddLink method to DummySpan #192

merged 3 commits into from
Apr 22, 2024

Conversation

vikmik
Copy link
Contributor

@vikmik vikmik commented Apr 18, 2024

This is only in case you'd rather not merge #191 because of the minimum go version bump.

As it stands, projects with holster as indirect dependency cannot build if they also use otel v1.25.0. See also #189
For example, a project using gubernator and otel 1.25.0 cannot build at the moment, because of this.

OTel added a new function to the Span interface, so this PR is the minimal change that can unblock things. I would prefer #191 to be merged, but either will work.

@vtopc vtopc requested a review from Baliedge April 19, 2024 07:56
tracing/dummy_span.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Baliedge Baliedge left a comment

Choose a reason for hiding this comment

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

I opt to approve this PR over #191 to solve the immediate need while avoiding the OTel upgrade complexities, which is a separate issue.

@vikmik
Copy link
Contributor Author

vikmik commented Apr 22, 2024

@vtopc @Baliedge sorry for the linting mishap - I've updated the PR, could you retrigger the workflows?

@vikmik
Copy link
Contributor Author

vikmik commented Apr 22, 2024

Thanks @vtopc - once this PR is merged it will unblock our project

@Baliedge Baliedge merged commit 249e239 into mailgun:master Apr 22, 2024
4 checks passed
@vtopc
Copy link
Contributor

vtopc commented Apr 22, 2024

Thanks @vtopc - once this PR is merged it will unblock our project

No. Thank you.

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.

3 participants