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

add option to skip log tokenization #23440

Merged

Conversation

haimrubinstein
Copy link
Contributor

Description: Performance improvement for UDP receiver.
Currently, UDP created on each log line a scanner object with split functions, even if there was no need for log tokenizing.
this PR adds a new config option 'tokenize_log' (default value set to true) which lets you decide if this is the behavior you want.
if set as false the scanner object is not created and the performance is increased significantly (up to 3X)

Link to tracking Issue: #23237

Testing:
Created a local build of the UDP receiver and tested that the logs arrive and exported as expected.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 18, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

I like how simple this solution is, but I wonder if we're applying it too specifically to the udp input. Should this also be a concern of tcp?

pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
@haimrubinstein haimrubinstein marked this pull request as ready for review June 28, 2023 09:07
@haimrubinstein haimrubinstein requested a review from a team June 28, 2023 09:07
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

@haim6678, please sign the CLA

@haimrubinstein
Copy link
Contributor Author

@djaglowski I added a change log file, can you re-approve, please?

.chloggen/add-udp-tokenize-flag.yaml Outdated Show resolved Hide resolved
.chloggen/add-udp-tokenize-flag.yaml Outdated Show resolved Hide resolved
@haimrubinstein
Copy link
Contributor Author

haimrubinstein commented Jul 2, 2023

I like how simple this solution is, but I wonder if we're applying it too specifically to the UDP input. Should this also be a concern of tcp?

I agree, I thought of this too. TCP input has the same logic so I assume it's suffering from the same issue. I can do it once this PR is merged if you want

@djaglowski
Copy link
Member

I like how simple this solution is, but I wonder if we're applying it too specifically to the UDP input. Should this also be a concern of tcp?

I agree, I thought of this too. TCP input has the same logic so I assume it's suffering from the same issue. I can do it once this PR is merged if you want

I think the two receivers are related enough that it would be best to keep them in sync. Do you mind adding the tcp changes to this PR?

@haimrubinstein
Copy link
Contributor Author

I like how simple this solution is, but I wonder if we're applying it too specifically to the UDP input. Should this also be a concern of tcp?

I agree, I thought of this too. TCP input has the same logic so I assume it's suffering from the same issue. I can do it once this PR is merged if you want

I think the two receivers are related enough that it would be best to keep them in sync. Do you mind adding the tcp changes to this PR?

done

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Thanks for iterating @haim6678. This looks good but I have a few nits.

We also need to document the new parameter in the receiver README and operator README.

pkg/stanza/operator/input/tcp/tcp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/tcp/tcp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/udp/udp.go Outdated Show resolved Hide resolved
pkg/stanza/operator/input/tcp/tcp.go Outdated Show resolved Hide resolved
@haimrubinstein
Copy link
Contributor Author

@djaglowski can you merge this PR? I'm not authorized.

@djaglowski
Copy link
Member

We also need to document the new parameter in the receiver README and operator README.

@haim6678, please also update the operator readmes

@haimrubinstein
Copy link
Contributor Author

We also need to document the new parameter in the receiver README and operator README.

@haim6678, please also update the operator readmes

done

@djaglowski djaglowski merged commit 7377f2a into open-telemetry:main Jul 7, 2023
88 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 7, 2023
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.

3 participants