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

Do not export empty span lists #896

Merged

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Mar 24, 2020

Fixes #895

@codecov-io
Copy link

codecov-io commented Mar 24, 2020

Codecov Report

Merging #896 into master will decrease coverage by 0.37%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
- Coverage   94.19%   93.81%   -0.38%     
==========================================
  Files         248      223      -25     
  Lines       10847     8793    -2054     
  Branches     1059      857     -202     
==========================================
- Hits        10217     8249    -1968     
+ Misses        630      544      -86     
Impacted Files Coverage Δ
...telemetry-tracing/src/export/BatchSpanProcessor.ts 95.74% <100.00%> (+0.09%) ⬆️
...try-tracing/test/export/BatchSpanProcessor.test.ts 100.00% <100.00%> (ø)
packages/opentelemetry-plugin-mysql/src/utils.ts 90.90% <0.00%> (-4.55%) ⬇️
packages/opentelemetry-resources/src/constants.ts 100.00% <0.00%> (ø)
packages/opentelemetry-plugin-mysql/src/version.ts 100.00% <0.00%> (ø)
...ckages/opentelemetry-plugin-mongodb/src/version.ts 100.00% <0.00%> (ø)
...kages/opentelemetry-node/test/registration.test.ts 100.00% <0.00%> (ø)
...ages/opentelemetry-resources/test/Resource.test.ts 100.00% <0.00%> (ø)
...opentelemetry-api/src/metrics/NoopMeterProvider.ts 100.00% <0.00%> (ø)
...lemetry-resources/test/resource-assertions.test.ts 100.00% <0.00%> (ø)
... and 37 more

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

lgtm

@mayurkale22
Copy link
Member

@pauldraper would be nice if you can review this one.

@mayurkale22 mayurkale22 added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Mar 25, 2020
@pauldraper
Copy link
Contributor

pauldraper commented Mar 25, 2020

Looks good.

I do think that it would be even better to replace setInterval with setTimeout, and invoke it only when there is something to send.

In-memory polling is poor citizenry for an application. On dedicated hardware (e.g. server), it's fine, but for other cases (e.g. a desktop app), it's impolite to constantly be using the CPU and preventing memory from paging to disk.

@dyladan
Copy link
Member Author

dyladan commented Mar 25, 2020

@pauldraper so instead of having a timer fire every x seconds no matter what, you would have the timer start when the first ended span is added to the list, then when the timer fires it exports all ended spans. There is then not another timer set until onEnd is called again?

@dyladan
Copy link
Member Author

dyladan commented Mar 25, 2020

@pauldraper just updated to remove the in-memory polling. PTAL

@pauldraper
Copy link
Contributor

LGTM

@dyladan
Copy link
Member Author

dyladan commented Mar 26, 2020

Re-requesting reviews because of the changes suggested by @pauldraper

@dyladan dyladan removed the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Mar 26, 2020
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Added one optional nit, otherwise logic lgtm.

@dyladan dyladan added the enhancement New feature or request label Mar 27, 2020
@dyladan dyladan merged commit c15f360 into open-telemetry:master Mar 27, 2020
@dyladan dyladan deleted the batch-empty-export branch March 27, 2020 12:52
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchSpanProcessor calls exporter with empty list
5 participants