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

Zipkin-specific observer support #157

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

Conversation

iamtakingiteasy
Copy link

@iamtakingiteasy iamtakingiteasy commented Jun 27, 2020

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

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observers cannot access native zipkin span instance
1 participant