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(instrumentation-fetch): perf observer bugs #1868

Merged
merged 4 commits into from
Jan 30, 2021

Conversation

mhennoch
Copy link
Contributor

Which problem is this PR solving?

  • PerformanceObserver entries are not actually used due to several bugs. performance.getEntriesByType fallback is used so things are not that broken.

Short description of the changes

  • Move observer.disconnect to the setTimeout so it is not disconnected before the 300ms timeout. XHR instrumentation seems to work fine.
  • Don't shadow entries variable inside perf observer callback, currently empty entry list is always returned.
  • Normalize spanUrl check inside perf observer callback as it can be relative but entry.name is always absolute. The same is already done in web.getResource. Same issue in XHR instrumentation, will fix it there also.
  • Added mock perf observer(not sure if this is the right way but couldn't come up with anything better) so that unit tests actually use it instead of fallback getEntries. Updated/added some tests.

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #1868 (25ab4bf) into main (de8c2be) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1868      +/-   ##
==========================================
- Coverage   92.65%   92.45%   -0.20%     
==========================================
  Files         174      174              
  Lines        6030     6034       +4     
  Branches     1278     1280       +2     
==========================================
- Hits         5587     5579       -8     
- Misses        443      455      +12     
Impacted Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 99.22% <100.00%> (+3.22%) ⬆️
...async-hooks/src/AsyncLocalStorageContextManager.ts 25.00% <0.00%> (-75.00%) ⬇️
...kages/opentelemetry-node/src/NodeTracerProvider.ts 96.55% <0.00%> (-3.45%) ⬇️
...etry-exporter-prometheus/src/PrometheusExporter.ts 90.76% <0.00%> (-1.54%) ⬇️
...ges/opentelemetry-instrumentation-http/src/http.ts 94.73% <0.00%> (-0.81%) ⬇️

const entries = list.getEntries() as PerformanceResourceTiming[];
entries.forEach(entry => {
if (entry.initiatorType === 'fetch' && entry.name === spanUrl) {
const perfObsEntries = list.getEntries() as PerformanceResourceTiming[];
Copy link
Member

Choose a reason for hiding this comment

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

Feels like we should have a separate PR to turn on no-shadow for eslint. A quick trial of it shows 101 instances in this repo.

Copy link
Member

Choose a reason for hiding this comment

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

If you think that would avoid more bugs, could you open an issue so someone can handle it at some point ? (i could have opened it but i don't really understand whats shadowing exactly)

this._addFinalSpanAttributes(span, response);

setTimeout(() => {
spanData.observer?.disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

This was really subtle; thanks for catching it.

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, nice findings

@obecny obecny added the bug Something isn't working label Jan 27, 2021
@vmarchaud vmarchaud merged commit f4e7115 into open-telemetry:main Jan 30, 2021
@mhennoch mhennoch deleted the fetch-abs-rel branch February 1, 2021 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants