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

adding zipkin open tracing #48

Merged
merged 1 commit into from
Apr 6, 2021
Merged

adding zipkin open tracing #48

merged 1 commit into from
Apr 6, 2021

Conversation

arun0009
Copy link
Contributor

@arun0009 arun0009 commented Mar 26, 2021

This PR should resolve #46

Adding Zipkin Middleware Functions (Server, Proxy) and util functions for Tracing.

Updating go versions, workflows etc.

@jcchavezs Would appreciate it if you can review this PR.

@carlosedp - Prometheus Metrics Test is failing? I upgraded all library versions and go version to 1.16 of echo-contrib in this PR (but when I reverted back to master version of go.mod prometheus test was still failing, last test).

README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #48 (eff9df4) into master (978e622) will increase coverage by 4.10%.
The diff coverage is 63.11%.

❗ Current head eff9df4 differs from pull request most recent head e7bc612. Consider uploading reports for the commit e7bc612 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   50.00%   54.10%   +4.10%     
==========================================
  Files           5        8       +3     
  Lines         326      438     +112     
==========================================
+ Hits          163      237      +74     
- Misses        153      182      +29     
- Partials       10       19       +9     
Impacted Files Coverage Δ
prometheus/prometheus.go 48.80% <0.00%> (+0.85%) ⬆️
zipkintracing/response_writer.go 16.00% <16.00%> (ø)
zipkintracing/tracing.go 68.75% <68.75%> (ø)
casbin/casbin.go 84.61% <100.00%> (+4.61%) ⬆️
pprof/pprof.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7944b61...e7bc612. Read the comment docs.

@arun0009
Copy link
Contributor Author

@carlosedp - I fixed the failing test, appreciate if you can review it?

@lammel @pafuent - Can you please review this PR?

prometheus/prometheus_test.go Outdated Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
@arun0009
Copy link
Contributor Author

@jcchavezs - Thanks for reviewing. If you think I resolved all the open comments, can you please approve?

@jcchavezs
Copy link

Just a very minor nit. No need to call it zipkintracing, I guess you thought of this because of jaegertracing but that is actually the name of the project. How about just using zipkin?

Otherwise, truly great work!

zipkintracing/README.md Outdated Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
zipkintracing/tracing.go Show resolved Hide resolved
zipkintracing/tracing.go Outdated Show resolved Hide resolved
@arun0009
Copy link
Contributor Author

arun0009 commented Apr 4, 2021

@aldas - can you please review, I think I resolved most of your comments & thanks for such a detailed explanation.

@arun0009
Copy link
Contributor Author

arun0009 commented Apr 4, 2021

Just a very minor nit. No need to call it zipkintracing, I guess you thought of this because of jaegertracing but that is actually the name of the project. How about just using zipkin?

Otherwise, truly great work!

@jcchavezs - I want to leave it as zipkintracing or else it will collide with zipkin-go's package zipkin

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@jcchavezs
Copy link

LGTM! Great work.

Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

I only have one request. Please split up the PR into the zipkin tracing related files (with this PR) and the project related changes (like ISSUE_TEMPLATE, stale bot), ... into another PR.

So only the README.md and zipkin* files should be merged with this PR.
As aldas approved already we are basically ready to merge after splitting.

@arun0009
Copy link
Contributor Author

arun0009 commented Apr 6, 2021

@lammel - Okay, force pushed just zipkin tracing changes. I'm going to send 2 additional PRs

@arun0009 arun0009 requested a review from lammel April 6, 2021 02:03
@arun0009 arun0009 mentioned this pull request Apr 6, 2021
@lammel lammel merged commit 304bbdf into labstack:master Apr 6, 2021
@lammel
Copy link
Contributor

lammel commented Apr 6, 2021

Great, thanks a lot @arun0009!

@jcchavezs
Copy link

@arun0009 do you mind adding this extension into this list? https://github.com/openzipkin/openzipkin.github.io/blob/master/_data/community_tracers_instrumentation.yml. Later this will show up in https://zipkin.io/pages/tracers_instrumentation.html

@arun0009
Copy link
Contributor Author

arun0009 commented Apr 6, 2021

done @jcchavezs

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.

Create zipkin middleware
5 participants