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: add keys operation to getter #1576

Merged
merged 11 commits into from
Oct 19, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Oct 7, 2020

Fixes #1486

Changes getter and setter to be an interface rather than a function so more operations may be added later.

@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #1576 into master will decrease coverage by 0.22%.
The diff coverage is 74.19%.

@@            Coverage Diff             @@
##           master    #1576      +/-   ##
==========================================
- Coverage   91.40%   91.18%   -0.23%     
==========================================
  Files         165      164       -1     
  Lines        5013     5024      +11     
  Branches     1023     1026       +3     
==========================================
- Hits         4582     4581       -1     
- Misses        431      443      +12     
Impacted Files Coverage Δ
...ackages/opentelemetry-shim-opentracing/src/shim.ts 87.70% <ø> (ø)
...y-api/src/context/propagation/TextMapPropagator.ts 14.28% <14.28%> (ø)
packages/opentelemetry-api/src/api/propagation.ts 56.25% <33.33%> (-1.33%) ⬇️
...telemetry-plugin-grpc-js/src/server/patchServer.ts 88.52% <50.00%> (-1.48%) ⬇️
packages/opentelemetry-plugin-grpc/src/grpc.ts 93.26% <75.00%> (-0.46%) ⬇️
packages/opentelemetry-api/src/api/global-utils.ts 100.00% <100.00%> (ø)
...i/src/context/propagation/NoopTextMapPropagator.ts 83.33% <100.00%> (ø)
...-core/src/context/propagation/B3MultiPropagator.ts 100.00% <100.00%> (ø)
...metry-core/src/context/propagation/B3Propagator.ts 100.00% <100.00%> (ø)
...core/src/context/propagation/B3SinglePropagator.ts 100.00% <100.00%> (ø)
... and 6 more

@dyladan dyladan linked an issue Oct 7, 2020 that may be closed by this pull request
* A setter is specified by the caller to define a specific method
* to set key/value pairs on the carrier within a propagator.
*/
export interface Setter<Carrier = any> {
Copy link
Member

Choose a reason for hiding this comment

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

should the interfaces lives in types file or maybe the Noop ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The interface was originally in its own file, but I think it makes sense to keep it more tightly coupled to the TextMapPropagator since this getter is specifically for this type of propagator. If other propagator types are added in the future like binary they may have different interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with keeping the interfaces together with class if they are not exported or keeping them in some general file like types.ts. Otherwise I would rather keep them in separate file or in general types.ts as it quickly gets messy later with any refactoring or when trying to find the desired interface.
Because of that:

If other propagator types are added in the future like binary they may have different interfaces.

Can you change names:

  • Setter into TextMapSetter
  • Getter into TextMapGetter
  • defaultGetter into defaultTextMapGetter

and then mentioned types.ts should become textMapTypes.ts ?
WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The TextMapPropagator is already just an interface. Won't it be weird to have a types file, but then also have some types (the TextMapPropagator interface) defined in a different file? I agree with the renaming though.

Copy link
Member

Choose a reason for hiding this comment

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

you right it is interface already I was blind :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I did the rename as I think that is a good idea to prevent future incompatibilities

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan merged commit 956604e into open-telemetry:master Oct 19, 2020
@dyladan dyladan deleted the getter-keys branch October 19, 2020 21:09
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* feat: add keys operation to getter

* chore: lint

* chore: rename getter/setter to TextMapGetter/Setter

* chore: lint

* chore: use setter in fetch plugin

* chore: lint

* chore: remove logs
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* feat: add keys operation to getter

* chore: lint

* chore: rename getter/setter to TextMapGetter/Setter

* chore: lint

* chore: use setter in fetch plugin

* chore: lint

* chore: remove logs
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…est in node@18 (open-telemetry#1576)

* chore(restify): upgrade restify to 9.1.0

* chore(restify): upgrade restify to 10.0.0 && re-enable node@18 test

* chore(restify): upgrade restify to 11.1.0

* chore(restify): re-enable test in tav

* chore(restify): update tav with corrent node.js version range check

* chore(restify): fix missing newline

* chore(restify): update tav to version 9.1.0 instead of 9.0.0

* chore: upgrade document and tav for restify

* chore: pin restify version

---------

Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
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.

TextMapPropagator Getter.keys
4 participants