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

feat: Integrate Zipkin tracer #832

Merged
merged 2 commits into from
Sep 24, 2021

Conversation

tobbbles
Copy link
Contributor

Updates the ory/x/tracing package to the latest version, configures the provider to parse config file and envvar configuration for Zipkin, integrates the tracer

Related issue(s)

Checklist

Further Comments

The go.mod replace directive for updating just the tracing package is horrible, but there is no straight upgrade path for the ory/x package, since it still relies on the viperx package. I'd be happy to discuss the semantics around this and oathkeeper

@tobbbles tobbbles requested a review from aeneasr as a code owner September 23, 2021 14:06
@codecov
Copy link

codecov bot commented Sep 23, 2021

Codecov Report

Merging #832 (c12c716) into master (7504e1e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
+ Coverage   62.39%   62.42%   +0.03%     
==========================================
  Files         102      102              
  Lines        4789     4793       +4     
==========================================
+ Hits         2988     2992       +4     
  Misses       1528     1528              
  Partials      273      273              
Impacted Files Coverage Δ
driver/configuration/provider_viper.go 86.59% <100.00%> (+0.21%) ⬆️
driver/registry_memory.go 89.91% <100.00%> (+0.04%) ⬆️

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 7504e1e...c12c716. Read the comment docs.

@aeneasr
Copy link
Member

aeneasr commented Sep 23, 2021

Wow! I didn’t know it was possible to update sub-packages like that!

@tobbbles
Copy link
Contributor Author

Wow! I didn’t know it was possible to update sub-packages like that!

@aeneasr This made me think, and unfortunately - it's not possible! 😞 The go build ignore the replace step, but go get errors out since x/tracing itself isn't a module :(

I've updated the PR to remove this replace, and instead only upgraded ory/x to v0.0.165, the last tag before viperx got removed.

To clarify, what's the plan for Oathkeeper going forward with the removal of viperx?

@aeneasr
Copy link
Member

aeneasr commented Sep 24, 2021

To clarify, what's the plan for Oathkeeper going forward with the removal of viperx?

We want to move it also to Koanf as it is a much better system but are lacking resources to do so, as it would culminate in a larger refactoring of the internals.

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.

2 participants