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

Infinite Tracing: batching and compression #1723

Merged
merged 4 commits into from
Jan 4, 2023
Merged

Infinite Tracing: batching and compression #1723

merged 4 commits into from
Jan 4, 2023

Conversation

fallwith
Copy link
Contributor

@fallwith fallwith commented Jan 4, 2023

  • reimplement gRPC compression, now that we understand that the minimal_stack configuration setting will disable all compression
  • enable 'high' compression by default
  • enable batching by default
  • make the compression level and batching toggle publicly configurable

* reimplement gRPC compression, now that we understand that the `minimal_stack` configuration setting will disable all compression
* enable 'high' compression by default
* enable batching by default
* make the compression level and batching toggle publicly configurable
the binding.pry instances were removed, but the 'require' was left
behind
COMPRESSION_LEVELS = %i[none low medium high].freeze
DEFAULT_COMPRESSION_LEVEL = :none
SETTINGS_BASE = {'grpc.enable_deadline_checking' => 0}.freeze
SETTINGS_COMPRESSION_DISABLED = SETTINGS_BASE.merge({'grpc.minimal_stack' => 1}).freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the content removed from this channel.rb file was simply moved to config.rb, as it is more appropriate to be considered configuration related.

Before this PR, everyone got both enable_deadline_checking' => 0 and minimal_stack' => 1. Now minimal_stack => 1 is not used when compression is desired, as we have learned it will disable compression.

'grpc-encoding' => 'gzip',
'grpc-accept-encoding' => ['gzip'],
'content-coding' => 'gzip',
'content-encoding' => 'gzip'}.freeze
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To align with our gist, use gzip specific metadata.

:type => Symbol,
:allowed_from_server => false,
:external => :infinite_tracing,
:description => "Configure the compression level for data sent to the Trace Observer\nMay be one of " \
"[none|low|medium|high]\nBy default, compression is not used (level = none)"
"[none|low|medium|high]\n'high' is the default. Set the level to 'none' to disable compression"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make the settings public, and with the first public offering have the defaults be compression high, batching enabled. (See SWAG doc for details)

@@ -67,6 +67,10 @@ def filter_file
opts.fetch(:file, nil)
end

def infinite_tracing_suite?
suite == 'infinite_tracing'
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this file, I wanted to be able to able to perform:

bundle exec rake test:multiverse[infinite_tracing,file=config_test,debug] 

but it was incorrectly scoping my config_test file filter to the test/multiverse/suites/infinite_tracing directory. Now using file= will work as desired for 8T tests.

Copy link
Contributor

@tannalynn tannalynn left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! Could we add a changelog entry for this though before we merge?

Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

This looks great! Could you add a CHANGELOG entry?

@kaylareopelle
Copy link
Contributor

This looks great! Could you add a CHANGELOG entry?

Whoops, Tanna beat me to it!

@fallwith
Copy link
Contributor Author

fallwith commented Jan 4, 2023

Everything looks good to me! Could we add a changelog entry for this though before we merge?

Sure thing. Added with bdf91aa

@github-actions
Copy link
Contributor

github-actions bot commented Jan 4, 2023

SimpleCov Report

Coverage Threshold
Line 93.2% 93%
Branch 84.48% 84%

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.

3 participants