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

[OE] Support compression for requests #6366

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Apr 8, 2024

Description

Support compressing traffic with opensearch.compression: true or opensearch.compress: 'gzip'.

Also, set compression in the Node server and OpenSearch traffic for CI.

Issues Resolved

#5296

Changelog

  • feat: Support compressing traffic with opensearch.compression: true or opensearch.compress: 'gzip'

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

ashwin-pc
ashwin-pc previously approved these changes Apr 8, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM

src/core/server/opensearch/opensearch_config.ts Outdated Show resolved Hide resolved
AMoo-Miki
AMoo-Miki previously approved these changes Apr 11, 2024
ashwin-pc
ashwin-pc previously approved these changes Apr 12, 2024
Copy link
Contributor

❌ Invalid Prefix For Manual Changeset Creation

Invalid description prefix. Found "feat". Only "skip" entry option is permitted for manual commit of changeset files.

If you were trying to skip the changelog entry, please use the "skip" entry option in the ##Changelog section of your PR description.

@github-actions github-actions bot added failed changeset Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry and removed failed changeset labels Apr 16, 2024
@github-actions github-actions bot removed the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label May 2, 2024
Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.69%. Comparing base (9ac5203) to head (840f186).
Report is 602 commits behind head on main.

Files with missing lines Patch % Lines
src/core/server/opensearch/client/client_config.ts 0.00% 1 Missing and 1 partial ⚠️
src/core/server/opensearch/opensearch_config.ts 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6366      +/-   ##
==========================================
- Coverage   67.70%   67.69%   -0.01%     
==========================================
  Files        3417     3417              
  Lines       66922    66928       +6     
  Branches    10888    10891       +3     
==========================================
  Hits        45310    45310              
- Misses      18966    18968       +2     
- Partials     2646     2650       +4     
Flag Coverage Δ
Linux_1 33.17% <16.66%> (-0.01%) ⬇️
Linux_2 55.58% <33.33%> (-0.01%) ⬇️
Linux_3 45.25% <16.66%> (-0.02%) ⬇️
Linux_4 34.84% <16.66%> (-0.01%) ⬇️
Windows_1 33.19% <16.66%> (-0.01%) ⬇️
Windows_2 55.55% <33.33%> (-0.01%) ⬇️
Windows_3 45.25% <16.66%> (-0.01%) ⬇️
Windows_4 34.84% <16.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kavilla kavilla added the v2.15.0 label May 2, 2024
? DEFAULT_COMPRESSION
: typeof rawConfig.compression === 'string'
? rawConfig.compression
: undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

An expression of rawConfig.compression = false gets out of this as undefined which is the wrong message out of this function (because it was defined). For the sanity of future devs, we should:

Suggested change
: undefined;
: false;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for the sanity of future devs, can we not use nested ternaries, especially without parens? calling a function with readable if statements isn't the worst thing imo :)

@BionIT
Copy link
Collaborator

BionIT commented Jun 5, 2024

@AMoo-Miki @ashwin-pc could you help take another look at this PR which is targeting 2.15?

@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Jun 7, 2024

@ashwin-pc, I think Rocky is tied up. If you agree with my suggestion, we can move this along.

@@ -120,6 +121,10 @@ export function parseClientOptions(config: OpenSearchClientConfig, scoped: boole
clientOptions.disablePrototypePoisoningProtection = config.disablePrototypePoisoningProtection;
}

if (config.compression) {
clientOptions.compression = config.compression;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to add a test for this given we updated the test to not do this by default?

? DEFAULT_COMPRESSION
: typeof rawConfig.compression === 'string'
? rawConfig.compression
: undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for the sanity of future devs, can we not use nested ternaries, especially without parens? calling a function with readable if statements isn't the worst thing imo :)

@ananzh
Copy link
Member

ananzh commented Jun 10, 2024

move to 2.16

@ananzh ananzh added v2.17.0 and removed v2.16.0 labels Jul 25, 2024
@ashwin-pc ashwin-pc added v2.18.0 and removed v2.17.0 labels Sep 12, 2024
@kavilla kavilla added v2.19.0 and removed v2.18.0 labels Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Resource Management] Compress traffic
8 participants