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: adding jaeger tracing schema, updating documentation #830

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

tobbbles
Copy link
Contributor

This PR updates the configuration schema to document the tracing block, currently with only support for Jaeger as per #376.

This PR addresses two pieces:

  1. It documents the Jaeger tracing functionality :D
  2. It enables the use of tracing in the configuration file. Although feat: Enable tracing #376 added this functionality, it was only possible to configure via environment variables and not a configuration file, due to being missing in the schema.

Related issue(s)

N/A

Checklist

Further Comments

This enables users to configure Jaeger tracing via their YAML config. However in future Oathkeeper should also support other kinda of tracing providers from github.com/ory/x/tracing for more completeness across all projects.

Note:
The tracing schema used does not reference github.com/ory/x/tracing/config.schema.json as Oathkeeper does not implement the full functionality. It is however copy and pasted from there with all non-Jaeger implementations removed.

@tobbbles tobbbles requested a review from aeneasr as a code owner September 22, 2021 12:44
Comment on lines 10 to 12
If file `$HOME/.oathkeeper.yaml` exists, it will be used as a configuration file
which supports all configuration settings listed below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this was removed from running npm run gen:config

Copy link
Member

Choose a reason for hiding this comment

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

Possible, we probably did not run it yet since the script was updated at some point.

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #830 (36ed605) into master (057293f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #830   +/-   ##
=======================================
  Coverage   62.47%   62.47%           
=======================================
  Files         102      102           
  Lines        4813     4813           
=======================================
  Hits         3007     3007           
  Misses       1531     1531           
  Partials      275      275           

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 057293f...36ed605. Read the comment docs.

@aeneasr
Copy link
Member

aeneasr commented Sep 29, 2021

Looks like this has some conflicts with #832 being merged - could you please take a look? :)

@tobbbles
Copy link
Contributor Author

Looks like this has some conflicts with #832 being merged - could you please take a look? :)

master is currently broken due to #832 upgrading ory/x and the docs generation not being able to succeed. #838 will address this, and once merged I can update and re-generate the documentation for this PR :)

@aeneasr
Copy link
Member

aeneasr commented Oct 3, 2021

Everything is merged :)

@tobbbles tobbbles force-pushed the docs/tracing branch 2 times, most recently from 93083dd to 79d6a0f Compare October 3, 2021 12:20
@tobbbles
Copy link
Contributor Author

tobbbles commented Oct 3, 2021

Everything is merged :)

Rebased! Some cleanup on indentation from #829

@aeneasr aeneasr merged commit 59871fc into ory:master Oct 7, 2021
@tobbbles tobbbles deleted the docs/tracing branch October 7, 2021 07:28
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