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(express): construct span names correctly with ignored middleware layers #452

Merged
merged 7 commits into from
Apr 27, 2021

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Apr 26, 2021

Which problem is this PR solving?

When middlewares are ignored the final span name is constructed from a concatenation of middleware paths. More here: #451

@seemk seemk requested a review from a team April 26, 2021 12:34
@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #452 (76406ea) into main (550e32c) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
+ Coverage   95.23%   95.32%   +0.09%     
==========================================
  Files         133      126       -7     
  Lines        8223     7959     -264     
  Branches      807      774      -33     
==========================================
- Hits         7831     7587     -244     
+ Misses        392      372      -20     
Impacted Files Coverage Δ
...ges/opentelemetry-host-metrics/test/metric.test.ts
packages/opentelemetry-host-metrics/src/version.ts
packages/opentelemetry-host-metrics/src/metric.ts
...ackages/opentelemetry-host-metrics/src/stats/si.ts
...ages/opentelemetry-host-metrics/src/BaseMetrics.ts
...ges/opentelemetry-host-metrics/src/stats/common.ts
packages/opentelemetry-host-metrics/src/enum.ts
...entelemetry-instrumentation-express/src/express.ts 94.78% <0.00%> (+0.09%) ⬆️
...instrumentation-express/test/custom-config.test.ts 98.85% <0.00%> (+0.51%) ⬆️

@seemk
Copy link
Contributor Author

seemk commented Apr 26, 2021

Hmm, suddenly unrelated CI errors after merging 5ecccc9

@Flarna
Copy link
Member

Flarna commented Apr 26, 2021

Most likely a side effect of DefinitelyTyped/DefinitelyTyped#52509
Maybe updating the dependencies of @opentelemetry/instrumentation-hapi solves this.

@dyladan
Copy link
Member

dyladan commented Apr 26, 2021

build works locally on my machine but seems to complain about hapi/podium only in CI. any ideas? is it building successfully for others locally?

seems to affect all PRs not just this one

edit: just saw your earlier message

@Flarna
Copy link
Member

Flarna commented Apr 26, 2021

a fix should be included in #455

@dyladan
Copy link
Member

dyladan commented Apr 26, 2021

Any idea why it works locally?

@Flarna
Copy link
Member

Flarna commented Apr 26, 2021

most likely because package-lock.json files are lying around and as a result transitive dependencies are not updated.
if I clean everything it fails locally.

@dyladan
Copy link
Member

dyladan commented Apr 26, 2021

same. i almost regret confirming this because now i can't build locally

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Approving but would like @vmarchaud approval before merge (original plugin author)

@vmarchaud vmarchaud merged commit 45e8751 into open-telemetry:main Apr 27, 2021
@vmarchaud
Copy link
Member

@seemk Thanks again for taking the time to do this and being patient with me ahah

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.

4 participants