[APR-258] Add option to compress logs with zstd #27148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv aws.create-vm --pipeline-id=53201356 --os-family=ubuntuNote: This applies to commit fdc16ca |
sgnn7
added
team/agent-processing-and-routing
and removed
team/agent-processing-and-routing
labels
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Go Package Import DifferencesBaseline: 9e36fae
|
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
StephenWakely
commented
Sep 4, 2024
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
Signed-off-by: Stephen Wakely <[email protected]>
ogaca-dd
requested changes
Jan 15, 2025
gh123man
reviewed
Jan 15, 2025
| CompressionLevel int `mapstructure:"compression_level" json:"compression_level"` | ||
| UseCompression bool `mapstructure:"use_compression" json:"use_compression"` | ||
| CompressionKind string `mapstructure:"compression_kind" json:"compression_kind"` | ||
| CompressionLevel int `mapstructure:"compression_level" json:"compression_level"` |
There was a problem hiding this comment.
Should we also have zstd_compression_level here? (and in the endpoint constructor below)
There was a problem hiding this comment.
CompressionLevel gets set to either compression_level or zstd_compression_level earlier on in the process since there's not a need to have two separate fields at this stage.
There was a problem hiding this comment.
Makes sense. I forget that additional_endpoints inherit the compression strategy from the main endpoint.
However this opens up a followup question, is there a use case where an additional endpoint would use a different compression strategy than the main endpoint? (ex if an on-prem log collector/proxy doesn't support zstd, and we enable zstd by default for the main endpoint).
We can probably track this as a followup.
There was a problem hiding this comment.
It's a good question. I think we have been looking towards only supporting one compression strategy throughout the agent (both metrics and logs). It just makes things simpler. So if there is a case where we should allow multiple at the same time, we should find out about these now.
Signed-off-by: Stephen Wakely <[email protected]>
AlexandreYang
approved these changes
Jan 16, 2025
ogaca-dd
approved these changes
Jan 16, 2025
Signed-off-by: Stephen Wakely <[email protected]>
remeh
reviewed
Jan 16, 2025
| @@ -474,6 +473,7 @@ func getSharedFxOption() fx.Option { | |||
| networkpath.Bundle(), | |||
| remoteagentregistryfx.Module(), | |||
| haagentfx.Module(), | |||
| metricscompressorfx.Module(), | |||
| ) | |||
There was a problem hiding this comment.
What's the reason we don't need the logscompression.Module() here? (while for instance we need it in the JMX command. FWIW it seems we need it in all other commands)
There was a problem hiding this comment.
It is being provided by process.Bundle(). It needs to be in this bundle, because that also includes the eventplatformimpl component which needs the logscompression component.
| @@ -125,7 +129,7 @@ func runAgent(tagger tagger.Component) { | |||
|
|
|||
| go startTraceAgent(&wg, lambdaSpanChan, coldStartSpanId, serverlessDaemon, tagger, rcService) | |||
| go startOtlpAgent(&wg, metricAgent, serverlessDaemon, tagger) | |||
| go startTelemetryCollection(&wg, serverlessID, logChannel, serverlessDaemon, tagger) | |||
| go startTelemetryCollection(&wg, serverlessID, logChannel, serverlessDaemon, tagger, compression) | |||
There was a problem hiding this comment.
Do you know how's serverless sending this telemetry? Through logs? Otherwise why are we providing a logs compression module here?
There was a problem hiding this comment.
I think so. It sets it up here.
Signed-off-by: Stephen Wakely <[email protected]>
StephenWakely
added
qa/rc-required
Signed-off-by: Stephen Wakely <[email protected]>
julien-lebot
approved these changes
Jan 16, 2025
remeh
approved these changes
Jan 16, 2025
daniel-taf
approved these changes
Jan 16, 2025
Signed-off-by: Stephen Wakely <[email protected]>
Serverless Benchmark Results
tl;drUse these benchmarks as an insight tool during development.
What is this benchmarking?The The benchmark is run using a large variety of lambda request payloads. In the charts below, there is one row for each event payload type. How do I interpret these charts?The charts below comes from The benchstat docs explain how to interpret these charts.
I need more helpFirst off, do not worry if the benchmarks are failing. They are not tests. The intention is for them to be a tool for you to use during development. If you would like a hand interpreting the results come chat with us in Benchmark stats |
gh123man
approved these changes
Jan 16, 2025
|
/merge |
Devflow running:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This adds a config option to compress log payloads with
zstd. We have a new option:compression_kinddefaults togzipif not specified. This was the only compression used previously.Motivation
Additional Notes
To make this change, in the interests of reusing code, I have moved the previous
gzipcode used for logs into thecomp/serializer/compressioncomponent. This component is currently used by metrics. I have had to add extra functionality to limit the available compression algorithms since metrics only allowszlibandzstdand logs only allowszstdandgzip.I'm not totally convinced that I have done this in a idiomatic way, so would appreciate feedback.
Note: We've discussed the compenitization with @DataDog/agent-shared-components , who wants some refactoring, but we'll do this after this is merged so that we can have zstd for logs available for testing.
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Configure the agent with:
Configure the agent to tail a file:
Add some text to
/tmp/zorkdork.log, ensure the logs are sent to Datadog and are decompressed correctly.QA for the open telemetry team
Can you make sure the otel collector can process logs correctly. Hopefully the
fxcomponents are wired up as you need them.