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

fix: move faas_id and cloud_account_id to semantic conventions #481

Merged
merged 3 commits into from May 12, 2021

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented May 11, 2021

Just a minor thing: in the aws-lambda instrumentation two attributes have not been moved to semantic conventions.

@svrnm svrnm requested a review from a team as a code owner May 11, 2021 13:05
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #481 (3ef166b) into main (da42d01) will increase coverage by 0.01%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #481      +/-   ##
==========================================
+ Coverage   95.23%   95.24%   +0.01%     
==========================================
  Files         130      134       +4     
  Lines        8325     8378      +53     
  Branches      811      814       +3     
==========================================
+ Hits         7928     7980      +52     
- Misses        397      398       +1     
Impacted Files Coverage Δ
...etapackages/auto-instrumentations-web/src/utils.ts 95.83% <ø> (ø)
...entelemetry-instrumentation-graphql/src/graphql.ts 92.25% <0.00%> (ø)
...opentelemetry-instrumentation-graphql/src/types.ts 100.00% <ø> (ø)
...ns/node/opentelemetry-instrumentation-pg/src/pg.ts 82.29% <ø> (ø)
...grpc-census-binary/test/BinaryTraceContext.test.ts 100.00% <ø> (ø)
...pc-census-binary/test/GrpcCensusPropagator.test.ts 92.07% <ø> (ø)
...opentelemetry-instrumentation-graphql/src/utils.ts 95.23% <100.00%> (ø)
...pentelemetry-instrumentation-pg/test/utils.test.ts 98.36% <100.00%> (ø)
...tor-grpc-census-binary/src/GrpcCensusPropagator.ts 97.43% <100.00%> (ø)
...metry-instrumentation-aws-lambda/src/aws-lambda.ts 94.11% <0.00%> (-0.07%) ⬇️
... and 8 more

@svrnm
Copy link
Member Author

svrnm commented May 11, 2021

Not sure why lint fails. The error and the suggested changes are not applied when I run npm run lint:fix locally and they are also reverted when I apply them manually locally

@dyladan
Copy link
Member

dyladan commented May 11, 2021

most likely a version issue. run lerna clean -y && rm -rf node_modules && git clean -fdx && npm i && npm run compile && npm run lint:fix then go make yourself a coffee

'faas.id': context.invokedFunctionArn,
[CLOUD_RESOURCE.ACCOUNT_ID]: AwsLambdaInstrumentation._extractAccountId(
[ResourceAttributes.FAAS_ID]: context.invokedFunctionArn,
[ResourceAttributes.CLOUD_ACCOUNT_ID]: AwsLambdaInstrumentation._extractAccountId(
Copy link
Member

Choose a reason for hiding this comment

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

FWIW this line is the one lint is complaining about most likely (too long)

@svrnm
Copy link
Member Author

svrnm commented May 11, 2021

@dyladan: I drank some coffee and run that command, thanks for the advice ☕ . Now I have 20 diffs after running npm lint:fix looks like the rules for the linter have changed.... ignore me ;-)

@svrnm
Copy link
Member Author

svrnm commented May 11, 2021

Ok, after cleaning up a few more things and making sure that (apparently) my repo is up to date, there are still 20 things to fix, so the rules of the linter have changed and those changes have not yet been applied:

svrnm@e7c9941

Since this is not scope of this PR, what's best practice, just have it here or open another PR?

@dyladan
Copy link
Member

dyladan commented May 11, 2021

it's fine to just check them in here

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

@vmarchaud vmarchaud merged commit 2e085fb into open-telemetry:main May 12, 2021
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

5 participants