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

[exporter/splunkhec] Allow to drop log messages longer than a given max length #18066

Closed
atoulme opened this issue Jan 27, 2023 · 13 comments · Fixed by #22984
Closed

[exporter/splunkhec] Allow to drop log messages longer than a given max length #18066

atoulme opened this issue Jan 27, 2023 · 13 comments · Fixed by #22984
Assignees
Labels
enhancement New feature or request exporter/splunkhec

Comments

@atoulme
Copy link
Contributor

atoulme commented Jan 27, 2023

Component(s)

exporter/splunkhec

Is your feature request related to a problem? Please describe.

Log records can have an indefinite size, and we want to make sure we don't overwhelm backends when we send logs.

Describe the solution you'd like

Allow the user to configure a max length for a log record of up to a fixed amount.
If a log record is longer than the max set, cut it into parts that fit in the limit, and add a field that identifies that this part of a series.

Describe alternatives you've considered

No response

Additional context

No response

@atoulme atoulme added enhancement New feature or request needs triage New item requiring triage labels Jan 27, 2023
@hvaghani221
Copy link
Member

Hey @atoulme, I am not 100% sure of the requirements here. We can break events to fix the max limit, but we can not merge those events again into splunk AFAIK.

From the description, it looks like you are asking to add a unique id (and maybe the sequence number) to those broken events to group them together in the search.

For example,

{"fields":{}, "event":"event1"}
{"fields":{}, "event":"event2"}

It will be converted to:

{"fields":{"id":"id-1", "seq-no":1}, "event":"event1.1"}
{"fields":{"id":"id-1", "seq-no":2}, "event":"event1.2"}
{"fields":{"id":"id-1", "seq-no":3}, "event":"event1.3"}
{"fields":{"id":"id-2", "seq-no":1}, "event":"event2.1"}
{"fields":{"id":"id-2", "seq-no":2}, "event":"event2.2"}

Let me know if I am not interpreting it correctly.

@github-actions
Copy link
Contributor

Pinging code owners for exporter/splunkhec: @atoulme @dmitryax. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@atoulme atoulme removed the needs triage New item requiring triage label Feb 8, 2023
@atoulme
Copy link
Contributor Author

atoulme commented Feb 8, 2023

No, we probably cannot merge them back, but I guess that is fine. Yes, the unique id and sequence number sounds good to me.

@shalper2
Copy link
Contributor

I'll take a crack at this using @harshit-splunk 's comment as a jumping off point

@atoulme
Copy link
Contributor Author

atoulme commented Mar 30, 2023

Talking about this with other folks, it appears splitting log messages should be done explicitly with a transformprocessor rather than try to do this in the exporter. We should abandon this approach.

@VihasMakwana
Copy link
Contributor

signalfx/splunk-otel-collector-chart#696 (comment)
this says to apply a 5Mb limit on the payload sent, we could add another config, max_payload_size or something.
If the final payload size is > max_payload_size, drop it.
This basically means implementing maxEventSize (from inputs.conf) in our exporter.
This way we wouldn't overwhelm the backends.

@VihasMakwana
Copy link
Contributor

@atoulme does this look good?
Can I raise a PR soon?

@atoulme
Copy link
Contributor Author

atoulme commented Apr 3, 2023

lgtm, go for it.

codeboten pushed a commit that referenced this issue Apr 14, 2023
Added a new config to apply the maxEventSize on our data.
- It applies this limit on uncompressed data

Link to tracking Issue: #18066

---------

Co-authored-by: Antoine Toulme <antoine@toulme.name>
@VihasMakwana
Copy link
Contributor

@atoulme I think we can close this one as PR is merged

@atoulme atoulme closed this as completed May 3, 2023
@dmitryax
Copy link
Member

@atoulme @VihasMakwana, this issue sounds a bit different from the implementation done in #20752:

The issue and the name of the configuration MaxEventSize are supposed to be applied to individual log records. But #20752 is implemented as another limit for a batch of logs, the same as MaxContentLengthLogs but for uncompressed data.

Do we really need another batch limit with a confusing name? I'd suggest we make it applied to individual log records and keep MaxContentLengthLogs as the only batch limit. That will simplify the logic and make the MaxEventSize name clear. I can apply the change if there are no objections

@atoulme
Copy link
Contributor Author

atoulme commented May 15, 2023

Ah, that is a bug then. MaxEventSize is only to catch events that are too long and has nothing to do with batching.

@atoulme
Copy link
Contributor Author

atoulme commented May 15, 2023

Yes, this is not implemented properly, apologies. https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/20752/files#r1194214914 has the offending line.

@atoulme
Copy link
Contributor Author

atoulme commented May 26, 2023

PR is out with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment