feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

legalimpurity · 2024-03-27T12:18:21Z

Which problem is this PR solving?

This is an implementation of JaegerRemoteSampler. This follows the specification mentioned here.

Fixes #4534

Short description of the changes

Add a new samper implementation in experimental packages.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Have added full coverage for entire code.

Checklist:

Followed the style guidelines of this project
Unit tests have been added

…legalimpurity/opentelemetry-js into feature/implement_jaeger_remote_sampler

codecov · 2024-03-27T15:37:21Z

Codecov Report

Attention: Patch coverage is 94.44444% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 92.86%. Comparing base (2610122) to head (7f476b3).
Report is 12 commits behind head on main.

❗ Current head 7f476b3 differs from pull request most recent head b553fbe. Consider uploading reports for the commit b553fbe to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4589      +/-   ##
==========================================
+ Coverage   90.77%   92.86%   +2.08%     
==========================================
  Files          90      331     +241     
  Lines        1930     9571    +7641     
  Branches      417     2054    +1637     
==========================================
+ Hits         1752     8888    +7136     
- Misses        178      683     +505

Files	Coverage Δ
...s/sampler-jaeger-remote/src/JaegerRemoteSampler.ts	`100.00% <100.00%> (ø)`
...s/sampler-jaeger-remote/src/PerOperationSampler.ts	`100.00% <100.00%> (ø)`
...mental/packages/sampler-jaeger-remote/src/types.ts	`100.00% <100.00%> (ø)`
...y-sdk-trace-base/src/sampler/ParentBasedSampler.ts	`78.78% <0.00%> (-5.09%)`	⬇️
...trace-base/src/sampler/TraceIdRatioBasedSampler.ts	`92.00% <0.00%> (-8.00%)`	⬇️

... and 244 files with indirect coverage changes

…ger_remote_sampler

legalimpurity · 2024-04-10T04:13:19Z

Good morning guys? did anyone had a chance to review this?

legalimpurity · 2024-04-18T04:48:28Z

Hello Guys any update here? @pichlermarc

pichlermarc

Ah sorry, I must've not posted my review yesterday 😞 - a few comments, mostly around testing/imports.

Once addressed, I'll do another pass to look more closely into the actual implementation 👍

pichlermarc · 2024-04-03T14:54:56Z

experimental/packages/sampler-jaeger-remote/package.json

@@ -0,0 +1,69 @@
+{
+  "name": "@opentelemetry/sampler-jaeger-remote",
+  "version": "0.0.1",


Suggested change

"version": "0.0.1",

"version": "0.50.0",

pichlermarc · 2024-04-17T14:17:54Z

experimental/CHANGELOG.md

@@ -9,6 +9,7 @@ All notable changes to experimental packages in this project will be documented
 ### :rocket: (Enhancement)

 * feat(otlp-transformer): consolidate scope/resource creation in transformer [#4600](https://github.com/open-telemetry/opentelemetry-js/pull/4600)
+* feat(jaeger-remote-samper): added support of jaeger-remote-samper [#4534](https://github.com/open-telemetry/opentelemetry-js/pull/4589)


Suggested change

* feat(jaeger-remote-samper): added support of jaeger-remote-samper [#4534](https://github.com/open-telemetry/opentelemetry-js/pull/4589)

* feat(jaeger-remote-sampler): added support of jaeger-remote-sampler [#4534](https://github.com/open-telemetry/opentelemetry-js/pull/4589)

please also add a link to the spec in a sub-list item below.
The commit scope we use is usually the package name which is sampler-jaeger-remote (different order of words) - so we may want to use this here.

pichlermarc · 2024-04-17T14:24:56Z

experimental/packages/sampler-jaeger-remote/README.md

@@ -0,0 +1 @@
+# Sample Jaeger Remote Sampler


This needs a lot more details (usage examples, etc.)
Please also add the same disclaimer as is found on the other packages (the one about the package being experimental and possible breaking changes).

pichlermarc · 2024-04-17T14:26:50Z

experimental/packages/sampler-jaeger-remote/package.json

+    "typescript": "4.4.4"
+  },
+  "peerDependencies": {
+    "@opentelemetry/api": "1.8.0",


API dependencies are usually a range of the earliest API version that it can be used with up until the most recent version. To find the exact supported versions please check which features from the API you're using and in which version they were introduced.

pichlermarc · 2024-04-17T14:27:38Z

experimental/packages/sampler-jaeger-remote/src/JaegerRemoteSampler.ts

+ * limitations under the License.
+ */
+
+import { Context } from '@opentelemetry/api/src/context/types';


Avoid deep imports.

Suggested change

import { Context } from '@opentelemetry/api/src/context/types';

import { Context } from '@opentelemetry/api';

pichlermarc · 2024-04-17T14:36:04Z

experimental/packages/sampler-jaeger-remote/src/PerOperationSampler.ts

+  TraceIdRatioBasedSampler,
+} from '@opentelemetry/sdk-trace-base';
+import { SpanKind } from '@opentelemetry/api/src/trace/span_kind';
+import { Link } from '@opentelemetry/api/src/trace/link';


Suggested change

import { Link } from '@opentelemetry/api/src/trace/link';

import { Link } from '@opentelemetry/api';

pichlermarc · 2024-04-17T14:37:51Z

experimental/packages/sampler-jaeger-remote/src/JaegerRemoteSampler.ts

+  }
+
+  toString(): string {
+    return `JaegerRemoteSampler{endpoint=${this._endpoint},${


would there ever be any sensitive data (token and things like this) in this._endpoint?

nope. just get endpoint with maybe service name.

pichlermarc · 2024-04-17T14:40:27Z

experimental/packages/sampler-jaeger-remote/src/PerOperationSampler.ts

+} from '@opentelemetry/sdk-trace-base';
+import { SpanKind } from '@opentelemetry/api/src/trace/span_kind';
+import { Link } from '@opentelemetry/api/src/trace/link';
+import { SpanAttributes } from '@opentelemetry/api/src/trace/attributes';


Suggested change

import { SpanAttributes } from '@opentelemetry/api/src/trace/attributes';

import { SpanAttributes } from '@opentelemetry/api';

pichlermarc · 2024-04-17T14:42:47Z

packages/opentelemetry-sdk-trace-base/src/sampler/ParentBasedSampler.ts

+  // Required for testing
+  getRootSampler(): Sampler {
+    return this._root;
+  }
+


I'd also like to avoid this if possible. This will actually add to the public API of the stable package. If exported it should serve a purpose other than testing.

pichlermarc · 2024-04-17T14:44:06Z

packages/opentelemetry-sdk-trace-base/src/sampler/TraceIdRatioBasedSampler.ts

+  // Required for testing
+  getRatio(): number {
+    return this._ratio;
+  }
+


I'd also like to avoid this if possible. This will actually add to the public API of the package. If exported it should serve a purpose other than testing.

…pen-telemetry#4605) * feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter * fixup! feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter

…y#4609)

…pen-telemetry#4620)

…metry#4624) Now that instrumentation-undici exists, point to it for users that want instrumentation of Node.js' fetch().

…lemetry#4627) This package accidentally got a dep and devDep on api-logs, at different versions. The result was it being pinned at the older version in package-lock. Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>

…pen-telemetry#4612) * fix(otlp-grpc-exporter-base): avoid TypeError on exporter shutdown * chore: update changelog * fix: use gRPC Client type over any * fixup! fix: use gRPC Client type over any * fix: use ts-lint/ts-ignore

…ger_remote_sampler

…te_sampler

legalimpurity added 9 commits March 14, 2024 17:53

Initial commit for Jaeger Remote Sampler

3a7418b

Implementation for Jaeger remote sampling done

8c6d1ef

adding more coverage

bed98fa

Merge branch 'main' into feature/implement_jaeger_remote_sampler

73799a1

more coverage wip

318c261

more coverage wip

fd18fb3

full coverage

4536d9e

reverted remove of tsconfig.tsbuildinfo

48aefc4

reverted remove of tsconfig.tsbuildinfo

caeaa83

legalimpurity requested a review from a team as a code owner March 27, 2024 12:18

legalimpurity and others added 5 commits March 27, 2024 17:48

Merge branch 'main' into feature/implement_jaeger_remote_sampler

b000047

lint fix

c3c2601

Merge branch 'feature/implement_jaeger_remote_sampler' of github.com:…

53ae858

…legalimpurity/opentelemetry-js into feature/implement_jaeger_remote_sampler

updated readme

1075653

added package-lock.json

c87718a

legalimpurity and others added 14 commits March 27, 2024 22:48

downgrading lerna to run with node 14

829a590

downgrading lerna to run with node 14

bbf5493

fixed imports

c44df07

Merge remote-tracking branch 'origin/main' into feature/implement_jae…

46fb577

…ger_remote_sampler

lint fix

3ba3cb4

updating package-lock.json

2db38f4

Merge branch 'main' into feature/implement_jaeger_remote_sampler

a6198b7

updating package-lock.json

5814861

merge main

97c70e8

merge main

d6b1765

updating package-lock.json

3ec6c6e

added changelog

7f476b3

typo fix

180ad48

merging main

c62dde6

legalimpurity and others added 3 commits April 4, 2024 23:39

Merge branch 'main' into feature/implement_jaeger_remote_sampler

9754732

Merge branch 'main' into feature/implement_jaeger_remote_sampler

17503ed

readding changelog, was accidentally removed from main merge

139c941

legalimpurity changed the title ~~feat(jaeger-remote-samper): Implement jaeger remote sampler~~ feat(jaeger-remote-sampler): Implement jaeger remote sampler Apr 10, 2024

pichlermarc reviewed Apr 18, 2024

View reviewed changes

pichlermarc and others added 9 commits April 26, 2024 09:09

feat(sdk-trace-base): log resource attributes in ConsoleSpanExporter (o…

d0e844d

…pen-telemetry#4605) * feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter * fixup! feat(sdk-trace-base): log resoruce attributes in ConsoleSpanExporter

feat(sdk-node): remove deprecated methods from NodeSDK (open-telemetr…

7cb4798

…y#4609)

test(expoerter-*-grpc): use RegExp to match Deadline Exceeded error (o…

ef08524

…pen-telemetry#4620)

doc(instr-fetch): mention instr-undici for Node.js fetch() (open-tele…

6f1e606

…metry#4624) Now that instrumentation-undici exists, point to it for users that want instrumentation of Node.js' fetch().

Merge remote-tracking branch 'origin/main' into feature/implement_jae…

5ba514f

…ger_remote_sampler

addressing review comments

413a8a1

Merge branch 'open-telemetry:main' into feature/implement_jaeger_remo…

b553fbe

…te_sampler

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

legalimpurity commented Mar 27, 2024 •

edited

codecov bot commented Mar 27, 2024 •

edited

legalimpurity commented Apr 10, 2024

legalimpurity commented Apr 18, 2024

pichlermarc left a comment

pichlermarc Apr 3, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

legalimpurity Apr 26, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

pichlermarc Apr 17, 2024

	* feat(jaeger-remote-samper): added support of jaeger-remote-samper [#4534](https://github.com/open-telemetry/opentelemetry-js/pull/4589)
	* feat(jaeger-remote-sampler): added support of jaeger-remote-sampler [#4534](https://github.com/open-telemetry/opentelemetry-js/pull/4589)

	import { Context } from '@opentelemetry/api/src/context/types';
	import { Context } from '@opentelemetry/api';

	import { Link } from '@opentelemetry/api/src/trace/link';
	import { Link } from '@opentelemetry/api';

	import { SpanAttributes } from '@opentelemetry/api/src/trace/attributes';
	import { SpanAttributes } from '@opentelemetry/api';

feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

Are you sure you want to change the base?

feat(jaeger-remote-sampler): Implement jaeger remote sampler #4589

Conversation

legalimpurity commented Mar 27, 2024 • edited

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Mar 27, 2024 • edited

Codecov Report

legalimpurity commented Apr 10, 2024

legalimpurity commented Apr 18, 2024

pichlermarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legalimpurity commented Mar 27, 2024 •

edited

codecov bot commented Mar 27, 2024 •

edited