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

updating to core v.0.13.0 #277

Merged
merged 14 commits into from Dec 17, 2020
Merged

updating to core v.0.13.0 #277

merged 14 commits into from Dec 17, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Dec 11, 2020

  • all needed changes to make it work with v0.13.0

Unresolved: opentelemetry-propagator-grpc-census-binary - TextMapGetter has values defined as text whereas this propagator expect data to be Buffer. Should we create a new getter or add this type to TextMapGetter ?
Resolved

@obecny obecny requested a review from a team as a code owner December 11, 2020 01:34
@obecny obecny changed the title v012 - updating to core v.0.13.0 updating to core v.0.13.0 Dec 11, 2020
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #277 (c41416e) into master (49db1b2) will increase coverage by 0.36%.
The diff coverage is 81.53%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
+ Coverage   94.95%   95.32%   +0.36%     
==========================================
  Files         115      115              
  Lines        5990     5948      -42     
  Branches      618      586      -32     
==========================================
- Hits         5688     5670      -18     
+ Misses        302      278      -24     
Impacted Files Coverage Δ
...pc-census-binary/test/GrpcCensusPropagator.test.ts 92.07% <68.00%> (-2.31%) ⬇️
...entelemetry-instrumentation-graphql/src/graphql.ts 91.39% <75.00%> (-0.98%) ⬇️
...lugin-react-load/src/BaseOpenTelemetryComponent.ts 96.98% <75.00%> (-0.48%) ⬇️
.../opentelemetry-plugin-pg-pool/test/pg-pool.test.ts 89.68% <91.66%> (-0.09%) ⬇️
...opentelemetry-instrumentation-graphql/src/utils.ts 96.22% <100.00%> (+0.04%) ⬆️
...s/node/opentelemetry-plugin-pg-pool/src/pg-pool.ts 88.88% <100.00%> (-0.25%) ⬇️
...ins/node/opentelemetry-plugin-pg-pool/src/utils.ts 92.30% <100.00%> (-1.03%) ⬇️
plugins/node/opentelemetry-plugin-pg/src/pg.ts 91.52% <100.00%> (-0.15%) ⬇️
plugins/node/opentelemetry-plugin-pg/src/utils.ts 98.43% <100.00%> (+1.42%) ⬆️
...ugins/node/opentelemetry-plugin-pg/test/pg.test.ts 94.37% <100.00%> (ø)
... and 15 more

@vmarchaud
Copy link
Member

Unresolved: opentelemetry-propagator-grpc-census-binary - TextMapGetter has values defined as text whereas this propagator expect data to be Buffer. Should we create a new getter or add this type to TextMapGetter ?

Does the spec require data to be a buffer ? Could we just propagate a base64 value of this buffer ?

@obecny
Copy link
Member Author

obecny commented Dec 11, 2020

Unresolved: opentelemetry-propagator-grpc-census-binary - TextMapGetter has values defined as text whereas this propagator expect data to be Buffer. Should we create a new getter or add this type to TextMapGetter ?

Does the spec require data to be a buffer ? Could we just propagate a base64 value of this buffer ?

I'm not changing anything in implementation or in the logic itself - previously it was possible but it was replaced with TextMapGetter which is the successor of the previous getter. But TextMapGetter seems not to be compatible to what was previously.

@obecny
Copy link
Member Author

obecny commented Dec 11, 2020

@dyladan you wrote the TextMapGetter, can you please check it ?

@dyladan
Copy link
Member

dyladan commented Dec 14, 2020

@dyladan you wrote the TextMapGetter, can you please check it ?

looking now

@obecny
Copy link
Member Author

obecny commented Dec 14, 2020

@open-telemetry/javascript-approvers no unresolved things, as talked with @dyladan just added the info there and keep it like this for now until that issue is resolved.

examples/express/client.js Show resolved Hide resolved
plugins/node/opentelemetry-plugin-ioredis/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mongodb/src/mongodb.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mongodb/src/mongodb.ts Outdated Show resolved Hide resolved
}
},
keys(carrier) {
if (carrier == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to use === here in my opinion, so the code is explicit and to prevent confusion with coercion.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is exactly the same as defaultTextMapGetter the only difference was that I have to call "carrier.get" https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-api/src/context/propagation/TextMapPropagator.ts#L124

Copy link
Member

Choose a reason for hiding this comment

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

@blumamir I agree that we should use strict checking by default however we didn't find a concensus on this, so for now we kinda allow both

Copy link
Member

Choose a reason for hiding this comment

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

cool, it was just a suggestion.
Using == works too :)

return value;
},
keys(carrier) {
if (carrier == null) {
Copy link
Member

Choose a reason for hiding this comment

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

also here, better to use === In my opinion

Copy link
Member Author

Choose a reason for hiding this comment

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

@obecny
Copy link
Member Author

obecny commented Dec 15, 2020

@blumamir updated the statuses code, should be fine, now, thx for spotting that

plugins/node/opentelemetry-plugin-dns/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mysql/src/mysql.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mysql/src/mysql.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mysql/src/mysql.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-mysql/test/mysql.test.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-pg/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-pg/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-pg/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-redis/src/utils.ts Outdated Show resolved Hide resolved
plugins/node/opentelemetry-plugin-redis/src/utils.ts Outdated Show resolved Hide resolved
@blumamir
Copy link
Member

@blumamir updated the statuses code, should be fine, now, thx for spotting that

@obecny
Sure, Thanks for taking the time to update the plugins.
Added few more places where StatusCode needs to be corrected

@obecny obecny force-pushed the v012 branch 10 times, most recently from 8caf3d4 to 0a06342 Compare December 15, 2020 21:51
@obecny obecny merged commit 19f4319 into open-telemetry:master Dec 17, 2020
@obecny obecny deleted the v012 branch December 17, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants