feat: add otelx package #479

icyphox · 2022-03-11T07:20:00Z

otelx wraps around go.opentelemetry.io/otel to simplify creating and using tracers. This is separate from the ory/x/tracing package as that supports a few more tracers (Zipkin, Instana, ...) which otelx does not, yet.

A note about the tests: the test for Jaeger spans simply spins up a UDP server and passes when data is received. While the byte data looks to be correct, I have not added code to unmarshal it. The reasoning behind this decision:

OpenTelemetry's compact Thrift generation is already tested
github.com/apache/thrift/lib/go/thrift is fairly complex and would take a lot of time to get setup, just for one test

Checklist

I have read the contributing guidelines
and signed the CLA.
I have referenced an issue containing the design document if my change
introduces a new feature.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added necessary documentation within the code base (if
appropriate).

Demonsthere

Hi there :)
Adding some initial review comments, keep up the good work!

configx/provider.go

configx/provider_test.go

otelx/config.go

otelx/middleware.go

otelx/otel.go

otelx/sql/instrumentedsql.go

popx/migrator.go

otelx wraps around go.opentelemetry.io/otel to simplify creating and using tracers.

Demonsthere · 2022-04-13T06:15:16Z

Great! It seems all that is left to do now i so to make the tests work :D https://github.com/ory/x/runs/5991843392?check_suite_focus=true#step%3A12%3A1202=

Demonsthere

LGTM :)

zepatrik

This looks pretty good already 👍
Can we remove the old tracing package as well?
Also, why not stick with the current local_agent_address config key to avoid breaking changes and all the edge cases in the first place?

configx/provider.go

otelx/config_test.go

otelx/jaeger.go

otelx/otel_test.go

otelx/sql/instrumentedsql.go

proxy/proxy.go

icyphox · 2022-04-14T12:56:11Z

Can we remove the old tracing package as well?

Done.

Also, why not stick with the current local_agent_address config key to avoid breaking changes and all the edge cases in the first place?

Also done. Not sure why that didn't occur to me 😅

zepatrik

Nice, this looks very good now 👍 Just some minor improvements 😉

otelx/config.schema.json

otelx/middleware.go

otelx/middleware_test.go

zepatrik

Just very minor improvements and we're good to go 🎉

zepatrik · 2022-04-19T06:01:24Z

otelx/config.schema.json

+                  "pattern": "^([0-9]{1,3}\\.){3}[0-9]{1,3}:([0-9]*)$"
+                },
+                {
+                  "format": "uri"


Format uri requires a scheme, but the code does not allow for parsing schemes, only hostname:port combinations.

See this example: https://go.dev/play/p/WS8_Bf_JRdR
I.e. for URIs to work, we have to use url.Parse

Yeah I get that, but removing "format": "urI" causes tests to fail and I'm not sure why.

otelx/config.schema.json

otelx/otel.go

popx/migrator.go

aeneasr · 2022-04-21T23:29:18Z

Guys, please don't remove packages just because a new package has been introduced. This makes it impossible to upgrade ory/x without changing all dependent code of the stuff that has been removed.

icyphox marked this pull request as ready for review March 14, 2022 11:19

icyphox marked this pull request as draft March 15, 2022 06:35

Demonsthere reviewed Apr 11, 2022

View reviewed changes

icyphox and others added 27 commits April 12, 2022 11:44

feat: add otelx package

2ec0e2d

otelx wraps around go.opentelemetry.io/otel to simplify creating and using tracers.

fix: configure agent host/port separately

809d5f4

fix: use int for port, update json-schema

084a78b

feat: add otelx package

281c6cc

otelx wraps around go.opentelemetry.io/otel to simplify creating and using tracers.

chore: remove all dependencies to older ory/x versions (#480)

3774af9

feat(configx): add config for otelx

6915ea0

test: config schema

95b83cb

test: add tests for jaeger spans

7a1e9a9

refactor(configx): switch to otelx

e2d2671

feat: add trace context propagation to tracer

5bf6176

feat: support for jaeger remote sampler

4590982

fix(configx): ensure tracer isn't nil

d191bbd

fix(configx): fix type assertion panic

036a4f0

feat: setup context propagation for b3 and jaeger

ee46db9

feat(httpx): use otelhttp as transport

259df3d

fix: import path

62dce7c

test(httpx): use otel.Tracer instead

f839bb3

fix: config handling

757679c

fix(configx): provider config

c64d6d9

test: fix config

88aa134

chore: bump opentelemetry version

7aa8e61

feat(networkx): use otelx

4cfd65e

feat(popx): use otelx

e7b47b8

fix: server url

6a3f1ce

chore: rename config option to 'tracing'

415d17c

fix: skip tracer setup when empty

d237db7

feat: configure jaeger remote sampler via env

4e4ac99

icyphox and others added 4 commits April 13, 2022 13:33

test: local agent address

f24f56d

fix: update schema loader

47a85d4

test: fix configx

15a2a98

refactor: don't use env vars

dfef73e

icyphox marked this pull request as ready for review April 13, 2022 13:28

Demonsthere approved these changes Apr 14, 2022

View reviewed changes

zepatrik requested changes Apr 14, 2022

View reviewed changes

icyphox added 7 commits April 14, 2022 16:50

chore: use only local agent address

49eff6c

test: add case for not health endpoint

7f97726

test: improve jaeger tracer tests

58172c3

test: use local agent address only

bc52b2e

chore: remove tracing package

c03bc98

chore: use same ctx var

b3c736c

chore: cleanup

0965cfa

zepatrik reviewed Apr 14, 2022

View reviewed changes

otelx/config.schema.json Outdated Show resolved Hide resolved

otelx/middleware.go Outdated Show resolved Hide resolved

otelx/middleware_test.go Show resolved Hide resolved

icyphox added 2 commits April 18, 2022 10:56

chore: simplify schema

140d1d5

chore: log address

0e9d5f2

zepatrik requested changes Apr 19, 2022

View reviewed changes

icyphox and others added 7 commits April 19, 2022 18:17

chore: update schema

af09c13

feat: use stringsx.SwitchExact

7bbd660

chore: cleanup

b6387ae

Merge branch 'master' of github.com:ory/x into feat/open-telemetry

5709320

chore: bump go version

649cf4e

chore: remove WrapHTTPRouter

fea7fea

fix: allow hostname and port combination for local_agent_address

73d266c

zepatrik approved these changes Apr 20, 2022

View reviewed changes

zepatrik merged commit 4bf4fdd into ory:master Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add otelx package #479

feat: add otelx package #479

icyphox commented Mar 11, 2022 •

edited

Loading

Demonsthere left a comment

Demonsthere commented Apr 13, 2022

Demonsthere left a comment

zepatrik left a comment

icyphox commented Apr 14, 2022

zepatrik left a comment

zepatrik left a comment

zepatrik Apr 19, 2022

zepatrik Apr 20, 2022

icyphox Apr 20, 2022

aeneasr commented Apr 21, 2022

feat: add otelx package #479

feat: add otelx package #479

Conversation

icyphox commented Mar 11, 2022 • edited Loading

Related Issue or Design Document

Checklist

Demonsthere left a comment

Choose a reason for hiding this comment

Demonsthere commented Apr 13, 2022

Demonsthere left a comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

icyphox commented Apr 14, 2022

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Apr 19, 2022

Choose a reason for hiding this comment

zepatrik Apr 20, 2022

Choose a reason for hiding this comment

icyphox Apr 20, 2022

Choose a reason for hiding this comment

aeneasr commented Apr 21, 2022

icyphox commented Mar 11, 2022 •

edited

Loading