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

feat: lazy initialization of the gzip stream #2581

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

fungiboletus
Copy link
Contributor

@fungiboletus fungiboletus commented Nov 2, 2021

Which problem is this PR solving?

A gzip stream is created by opentelemetry-exporter-otlp-http even when it's never used. Moreover, it's also never ended. It's particularly visible when using the testing framework Jest, that will complain about it with the following message : Jest has detected the following 1 open handle potentially keeping Jest from exiting: ZLIB.

Short description of the changes

Having a way to call gzip.end() would fix the issue, but I think it's first preferable to avoid creating the gzip stream in the first place.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I ran npm test in the root folder and experimental folder.

Checklist:

  • Followed the style guidelines of this project
    - [ ] Unit tests have been added (not required IMHO)
    - [ ] Documentation has been updated (not required)

@fungiboletus fungiboletus requested a review from a team as a code owner November 2, 2021 10:36
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 2, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #2581 (4bb0158) into main (1abda14) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2581   +/-   ##
=======================================
  Coverage   93.00%   93.00%           
=======================================
  Files         138      138           
  Lines        5092     5092           
  Branches     1095     1095           
=======================================
  Hits         4736     4736           
  Misses        356      356           

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.

We should add end() to the shutdown method of the exporter as well. Can you please add a todo comment so we don't forget?

@dyladan
Copy link
Member

dyladan commented Nov 2, 2021

Please fix the conflicts

@fungiboletus
Copy link
Contributor Author

Conflicts are fixed and I added the comment. I'm not sure how you do about the merge commit because I can't see any in the git log of the main branch, but perhaps you rebase everytime.

@dyladan
Copy link
Member

dyladan commented Nov 2, 2021

All PRs are squashed when we merge. That's why they have to be up to date. Speaking of which, this PR is out of date so it needs to be updated in order to be mergeable.

@dyladan
Copy link
Member

dyladan commented Nov 8, 2021

Re-running tests to see if failure is persistent.

@dyladan
Copy link
Member

dyladan commented Nov 8, 2021

Looks like the error wasn't caused by this PR. Please update the branch so it can be merged.

@dyladan dyladan added the enhancement New feature or request label Nov 8, 2021
@dyladan dyladan changed the title perf: lazy initialization of the gzip stream feat: lazy initialization of the gzip stream Nov 8, 2021
@dyladan
Copy link
Member

dyladan commented Nov 9, 2021

Please update the branch so this can be merged. I want to cut a release and I'd like this to be in it.

@fungiboletus
Copy link
Contributor Author

I'm working on it now.

@fungiboletus
Copy link
Contributor Author

Is the merge enough or should I do something else ?

@dyladan
Copy link
Member

dyladan commented Nov 9, 2021

No thats fine as long as the tests pass

@dyladan dyladan merged commit bc50c7b into open-telemetry:main Nov 9, 2021
@dyladan dyladan added bug Something isn't working and removed enhancement New feature or request labels Nov 9, 2021
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.

None yet

6 participants