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.

Sign up for GitHub

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

Jump to bottom

Zipkin-specific observer support #157

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Zipkin-specific observer support #157

wants to merge 1 commit into from

Conversation

iamtakingiteasy
Copy link

@iamtakingiteasy iamtakingiteasy commented Jun 27, 2020 •
edited
Loading

Naive approach to adding support for zipkin-specific observers.

Merging two interfaces does not seem to be viable, either type-safety- or performance-wise.

closes #156

@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch from 262d7ed to e36ac10 Compare June 27, 2020 20:01
@iamtakingiteasy
Copy link
Author

iamtakingiteasy commented Jun 27, 2020 •
edited
Loading

Fixed newly-introduced golangci-lint complaints regarding repeating string constants in test files in last force push.

There is still two complaints from upstream remaining, regarding parseTagsAsZipkinOptions cyclomatic complexity and benchmarkWithOps's numItems parameter always being 0.

Possibly it is worth disabling golangci-lint for *_test.go files and either splitting parseTagsAsZipkinOptions in few further functions, or raising gocyclo limit and/or using gocognit instead.

@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch from e36ac10 to 68f9fd6 Compare June 27, 2020 20:27
@iamtakingiteasy
Copy link
Author

Cleaned up implementation a bit.

@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch 3 times, most recently from 6ec8c85 to 30d9174 Compare June 27, 2020 21:35
@iamtakingiteasy
Copy link
Author

Added more test-cases.

@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch 2 times, most recently from 6cb8f48 to 5ed71da Compare June 28, 2020 02:35
@iamtakingiteasy
Copy link
Author

iamtakingiteasy commented Jun 28, 2020 •
edited
Loading

Removed redundant FinisherWithDuration interface, as zipkin.Span already provides that method.
This in turns makes clear that FinishWithOptions() cannot return without actually finishing the span.

Added SetRemoteEndpoint in SetTag() as well as OnSetRemoteEndpoint() for ZipkinSpanObserver.

@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch 2 times, most recently from 7422fd1 to b1c5a41 Compare June 28, 2020 02:50
@iamtakingiteasy iamtakingiteasy force-pushed the 156-zipkin-specific-observer-support branch from b1c5a41 to 7eae14b Compare June 28, 2020 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Observers cannot access native zipkin span instance
1 participant
@iamtakingiteasy