chore: adopt opentelemetry-propagator-jaeger #1931

jtmalinowski · 2021-02-15T19:12:32Z

Move opentelemetry-propagator-jaeger from contrib to core.
This is to enable turning on the jaeger propagator using ENV variables.
Default Node Tracer Provider needs to depend on Jaeger propagator.

Which problem is this PR solving?

Progress towards General environment configuration #1675
unblocks feat: enable propagators using ENV vars #1929
chore: move opentelemetry-propagator-jaeger to core opentelemetry-js-contrib#351

Short description of the changes

moves opentelemetry-propagator-jaeger from contrib into core, allowing other core packages to rely on it

codecov · 2021-02-15T19:22:39Z

Codecov Report

Merging #1931 (ab14ebc) into main (a707588) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head ab14ebc differs from pull request most recent head 550ae3f. Consider uploading reports for the commit 550ae3f to get more accurate results

@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
- Coverage   93.00%   92.77%   -0.24%     
==========================================
  Files         152      146       -6     
  Lines        5944     5547     -397     
  Branches     1237     1161      -76     
==========================================
- Hits         5528     5146     -382     
+ Misses        416      401      -15

Impacted Files	Coverage Δ
...propagator-jaeger/src/JaegerHttpTracePropagator.ts	`100.00% <100.00%> (ø)`
...mentation-xml-http-request/src/enums/EventNames.ts
...ntelemetry-web/src/enums/PerformanceTimingNames.ts
...ackages/opentelemetry-web/src/WebTracerProvider.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
packages/opentelemetry-web/src/utils.ts
...kages/opentelemetry-web/src/StackContextManager.ts
packages/opentelemetry-web/src/types.ts

vmarchaud · 2021-02-15T19:51:17Z

Default Node Tracer Provider needs to depend on Jaeger propagator.

Sorry but i can't find in the spec where they require to install jeager propagator by default on the node tracer, unless i misunderstood ?

jtmalinowski · 2021-02-15T19:55:37Z

@vmarchaud so the basic requirements are here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#general-sdk-configuration, see "Known values for OTEL_PROPAGATORS are:". We have discussed a couple of approaches back in early January and we never managed to come up with a good optional dependency approach. E.g. peerdependencies generates warnings, so in the end we decided to just pull it in. Let me know if it's an issue.
Edit: in general for this ENV based configuration to work, we need either:

propagator packages to depend on a "propagators store", e.g. opentelemetry-core - I think someone was against that, but I can't remember why
or: the package with the Tracer to depend on on propagator packages

vmarchaud · 2021-02-15T20:58:43Z

Well i'm not sure if that means the sdk need to be able to instantiate all of them ? I mean there are values for vendor specific propagator

jtmalinowski · 2021-02-16T13:09:08Z

@vmarchaud Thanks for the time to review! Last time it was discussed there were no 3rd party propagators, now there are. 1st party propagators in core and loading out of the box seems like a good user experience to me. I'm not sure what to do about 3rd party propagators. Maybe it's best to quickly discuss this during SIG?

dyladan · 2021-02-16T13:33:54Z

Maybe it's best to quickly discuss this during SIG?

Can you please add it to the agenda?

vmarchaud · 2021-02-16T14:51:04Z

1st party propagators in core and loading out of the box seems like a good user experience to me

Agree, lets discuss this at the SIG indeed

MSNev

Is it possible to try doing this while preserving the history? I've never done this and it doesn't look like fun, but it would be "nice".

https://medium.com/@ayushya/move-directory-from-one-repository-to-another-preserving-git-history-d210fa049d4b

dyladan · 2021-02-19T21:25:00Z

Is it possible to try doing this while preserving the history? I've never done this and it doesn't look like fun, but it would be "nice".

medium.com/@ayushya/move-directory-from-one-repository-to-another-preserving-git-history-d210fa049d4b

I have done this multiple times. The best way I have found to do it is to use git-filter-repo, then you rebase that onto the new repository.

get a fresh clone of the source repo
use git-filter-repo to filter the source repo down to only the files you need
- git-filter-repo --path propagators/opentelemetry-propagator-jaeger --path-rename propagators/opentelemetry-propagator-jaeger:packages/opentelemetry-propagator-jaeger
add the target repo as a remote on your source clone
- git remote add core [email protected]:open-telemetry/opentelemetry-js.git
rebase the source on the target main
- git fetch core
- git rebase core/main
  - during this step you may need to use the -i flag to edit any commits with invalid CLA
push the source branch to a new branch on the target
- git push core HEAD:jaeger
Create a pull request from that branch to main
Create new commits on the branch for any compatibility changes that need to be made like the tsconfig references and such.

The biggest issue is ensuring all commit authors have signed the CLA. It shouldn't be a problem for us though because it is our repo.

I've also done the filter-branch method from that blog post and it is indeed not fun.

vmarchaud · 2021-02-24T09:16:16Z

Seeing the result of importing the history i would prefer to not import it at all, we will have release commit from contrib inside the core which i think will give us trouble later to understand what happened. Since all commits reference their PR for the context, i think it should be enough to squash everything.

dyladan · 2021-02-24T18:38:59Z

Seeing the result of importing the history i would prefer to not import it at all, we will have release commit from contrib inside the core which i think will give us trouble later to understand what happened. Since all commits reference their PR for the context, i think it should be enough to squash everything.

Squash looses the blames and the dates things happened which can be useful when debugging issues later. I personally use blame all the time to figure out who to ask about certain things. The release commits are different because these will be the release dates of the jaeger propagator (contrib) not the core repo. I think we can keep everything.

vmarchaud · 2021-03-01T19:53:43Z

@jtmalinowski I think we can move this forward, could you upgrade to use the version 0.18.0 of the API as the rest of the repo ?

vmarchaud · 2021-03-15T12:51:25Z

We need to merge this before merging open-telemetry/opentelemetry-js-contrib#351 so the propagator exists in one of our repo.

cc @open-telemetry/javascript-approvers can someone review this ? This is a simple cp from the contrib (see open-telemetry/opentelemetry-js-contrib#351)

obecny · 2021-03-15T13:17:12Z

packages/opentelemetry-propagator-jaeger/README.md

@@ -0,0 +1,63 @@
+# OpenTelemetry Propagator Jaeger
+
+[![Gitter chat][gitter-image]][gitter-url]


pls remove it

obecny · 2021-03-15T13:17:32Z

packages/opentelemetry-propagator-jaeger/README.md

+
+* For more information on OpenTelemetry, visit: <https://opentelemetry.io/>
+* For more about OpenTelemetry JavaScript: <https://github.com/open-telemetry/opentelemetry-js>
+* For help or feedback on this project, join us on [gitter][gitter-url]


pls remove it

obecny · 2021-03-15T13:18:04Z

packages/opentelemetry-propagator-jaeger/README.md

+
+[gitter-image]: https://badges.gitter.im/open-telemetry/opentelemetry-js.svg
+[gitter-url]: https://gitter.im/open-telemetry/opentelemetry-node?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge
+[license-url]: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/LICENSE


wrong link, please update he rest links too so they point to core repo

obecny · 2021-03-15T13:18:52Z

packages/opentelemetry-propagator-jaeger/karma.conf.js

@@ -0,0 +1,24 @@
+/*!
+ * Copyright 2020, OpenTelemetry Authors


please fix the headline

obecny · 2021-03-15T13:19:11Z

packages/opentelemetry-propagator-jaeger/package.json

+  "author": "OpenTelemetry Authors",
+  "license": "Apache-2.0",
+  "engines": {
+    "node": ">=8.0.0"


There seems to be a number of places where 8.0.0 is still specified (e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-jaeger/package.json#L29), is there some kind of a rule for this? Anyway, fixed!

When we started the project we simply started with node 8.0.0 support because that was the same as opencensus, however at some point we started to use perf_hooks which is only available starting 8.5.0. We then choose to up the minimum version for all new packages to 0.8.5 to keep consistency across the core packages (then same logic for contribs).

packages/opentelemetry-propagator-jaeger/.eslintrc.js

packages/opentelemetry-propagator-jaeger/package.json

dyladan · 2021-03-24T15:48:49Z

@jtmalinowski are you still working on this? There are several open comments over a week old

jtmalinowski · 2021-03-24T16:23:30Z

@dyladan To be honest, I don't think I'll have enough time to ensure smooth progress of this feature (specification env vars) within the next couple of weeks, so it's probably best if you reassign it to someone else. Unless you're fine if it's resumed in late April.

dyladan · 2021-03-24T16:53:41Z

it's fine if you don't have time for the ENV var feature, but we should at least try to bring the jaeger package across with no other changes yes?

jtmalinowski · 2021-03-25T12:59:50Z

@dyladan Sure, that makes sense. I'll address the comments within 2-3 days.

…propagator

jtmalinowski · 2021-03-28T22:03:52Z

Apologies, I had to force-push, because someone merged main in the meantime and there would otherwise be quite a lot of git history straightening for me locally.

@dyladan in terms of final commit structure, because we can't squash-merge for linear history, I suggest:

one final interactive rebase right before merging
interactive rebase to remove "merge main" commits
squash all my changes into 1 commit
merge PR using rebase-merge

If you don't mind non-linear history then just rebase-merge should be fine.

dyladan · 2021-03-29T14:06:57Z

I don't mind nonlinear history. I'm more worried about accurate blames than the timeline.

obecny

lgtm

Move opentelemetry-propagator-jaeger from contrib to core (open-telemetry#1931). This is to enable turning on the jaeger propagator using ENV variables. Default Node Tracer Provider needs to depend on Jaeger propagator.

jtmalinowski requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners February 15, 2021 19:12

jtmalinowski mentioned this pull request Feb 15, 2021

chore: move opentelemetry-propagator-jaeger to core open-telemetry/opentelemetry-js-contrib#351

Merged

jtmalinowski force-pushed the chore/adopt-jaeger-propagator branch from 3460398 to ca882b1 Compare February 15, 2021 19:32

MSNev reviewed Feb 18, 2021

View reviewed changes

jtmalinowski force-pushed the chore/adopt-jaeger-propagator branch from ca882b1 to 4dd4427 Compare February 23, 2021 22:18

vmarchaud mentioned this pull request Mar 1, 2021

propagator-jaeger: 16 character trace-id isn't zero-padded to 32 characters as per specification #1983

Closed

dyladan added 3 commits March 8, 2021 15:04

Move jaeger propagator to propagators folder

5131ee6

chore: fix build and link

e29e3dd

chore: force ci build

c63d7af

obecny reviewed Mar 15, 2021

View reviewed changes

Flarna reviewed Mar 15, 2021

View reviewed changes

vmarchaud self-requested a review March 20, 2021 18:07

jtmalinowski added 3 commits March 28, 2021 23:37

fix: adapt propagator-jaeger to core structure

047f7c2

Merge remote-tracking branch 'upstream/main' into chore/adopt-jaeger-…

2d73ee3

…propagator

chore: upgrade dependencies in propagator-jaeger

f92a2db

jtmalinowski force-pushed the chore/adopt-jaeger-propagator branch from e680d9c to f92a2db Compare March 28, 2021 21:57

jtmalinowski requested review from Flarna, obecny and MSNev March 28, 2021 22:14

Flarna approved these changes Mar 28, 2021

View reviewed changes

johnbley approved these changes Mar 29, 2021

View reviewed changes

dyladan approved these changes Mar 29, 2021

View reviewed changes

vmarchaud approved these changes Mar 29, 2021

View reviewed changes

obecny approved these changes Mar 29, 2021

View reviewed changes

vmarchaud mentioned this pull request Mar 29, 2021

chore: v0.18.1 release proposal #2052

Merged

Merge branch 'main' into chore/adopt-jaeger-propagator

550ae3f

dyladan closed this Mar 29, 2021

dyladan reopened this Mar 29, 2021

dyladan merged commit 4fc4a85 into open-telemetry:main Mar 29, 2021

dyladan added the internal label Mar 29, 2021

DTPierreGutierrez mentioned this pull request May 29, 2024

[Snyk] Upgrade @opentelemetry/context-zone from 0.11.0 to 0.26.0 DTPierreGutierrez/opentelemetry-examples#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: adopt opentelemetry-propagator-jaeger #1931

chore: adopt opentelemetry-propagator-jaeger #1931

jtmalinowski commented Feb 15, 2021 •

edited

Loading

codecov bot commented Feb 15, 2021 •

edited

Loading

vmarchaud commented Feb 15, 2021

jtmalinowski commented Feb 15, 2021 •

edited

Loading

vmarchaud commented Feb 15, 2021 •

edited

Loading

jtmalinowski commented Feb 16, 2021

dyladan commented Feb 16, 2021

vmarchaud commented Feb 16, 2021

MSNev left a comment

dyladan commented Feb 19, 2021 •

edited

Loading

vmarchaud commented Feb 24, 2021

dyladan commented Feb 24, 2021

vmarchaud commented Mar 1, 2021

vmarchaud commented Mar 15, 2021

obecny Mar 15, 2021

obecny Mar 15, 2021

obecny Mar 15, 2021

obecny Mar 15, 2021

obecny Mar 15, 2021

jtmalinowski Mar 28, 2021

vmarchaud Mar 29, 2021

dyladan commented Mar 24, 2021

jtmalinowski commented Mar 24, 2021

dyladan commented Mar 24, 2021

jtmalinowski commented Mar 25, 2021

jtmalinowski commented Mar 28, 2021

dyladan commented Mar 29, 2021

obecny left a comment

		@@ -0,0 +1,63 @@
		# OpenTelemetry Propagator Jaeger

		[![Gitter chat][gitter-image]][gitter-url]

chore: adopt opentelemetry-propagator-jaeger #1931

chore: adopt opentelemetry-propagator-jaeger #1931

Conversation

jtmalinowski commented Feb 15, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Feb 15, 2021 • edited Loading

Codecov Report

vmarchaud commented Feb 15, 2021

jtmalinowski commented Feb 15, 2021 • edited Loading

vmarchaud commented Feb 15, 2021 • edited Loading

jtmalinowski commented Feb 16, 2021

dyladan commented Feb 16, 2021

vmarchaud commented Feb 16, 2021

MSNev left a comment

Choose a reason for hiding this comment

dyladan commented Feb 19, 2021 • edited Loading

vmarchaud commented Feb 24, 2021

dyladan commented Feb 24, 2021

vmarchaud commented Mar 1, 2021

vmarchaud commented Mar 15, 2021

obecny Mar 15, 2021

Choose a reason for hiding this comment

obecny Mar 15, 2021

Choose a reason for hiding this comment

obecny Mar 15, 2021

Choose a reason for hiding this comment

obecny Mar 15, 2021

Choose a reason for hiding this comment

obecny Mar 15, 2021

Choose a reason for hiding this comment

jtmalinowski Mar 28, 2021

Choose a reason for hiding this comment

vmarchaud Mar 29, 2021

Choose a reason for hiding this comment

dyladan commented Mar 24, 2021

jtmalinowski commented Mar 24, 2021

dyladan commented Mar 24, 2021

jtmalinowski commented Mar 25, 2021

jtmalinowski commented Mar 28, 2021

dyladan commented Mar 29, 2021

obecny left a comment

Choose a reason for hiding this comment

jtmalinowski commented Feb 15, 2021 •

edited

Loading

codecov bot commented Feb 15, 2021 •

edited

Loading

jtmalinowski commented Feb 15, 2021 •

edited

Loading

vmarchaud commented Feb 15, 2021 •

edited

Loading

dyladan commented Feb 19, 2021 •

edited

Loading