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

Keda integration Logs #19460

Merged
merged 20 commits into from
Feb 10, 2025
Merged

Keda integration Logs #19460

merged 20 commits into from
Feb 10, 2025

Conversation

steveny91
Copy link
Contributor

@steveny91 steveny91 commented Jan 22, 2025

What does this PR do?

Logs pipeline for Keda

https://datadoghq.atlassian.net/browse/LOI-379

@steveny91 steveny91 requested review from a team as code owners January 22, 2025 21:36
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (b3da55a) to head (d613b74).
Report is 3 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
hive ?
hivemq ?
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
keda 88.05% <ø> (ø)
presto ?
solr ?

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

@steveny91 steveny91 requested review from a team as code owners January 22, 2025 21:49
@steveny91 steveny91 requested a review from a team January 22, 2025 21:49
Copy link
Contributor

@brunorenier brunorenier left a comment

Choose a reason for hiding this comment

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

There are quite a few places where we can refactor the grok expression to rely less on data. This is highly recommended as expressions with many data or expensive regexes might lead to the parsing being aborted if too inefficient.

@steveny91
Copy link
Contributor Author

@brunorenier Thanks for all the help so far! I really appreciate your input. I incorporated almost all of your suggestions, the only one I didn't I ended up dropping that rule and replacing it with something else. I think the only data matchers left are the ones for the keyvalues. Not sure if you have any ideas on how to address those. Thanks in advance and I do apologize for my shallow knowledge on the matter.

@brunorenier
Copy link
Contributor

@brunorenier Thanks for all the help so far! I really appreciate your input. I incorporated almost all of your suggestions, the only one I didn't I ended up dropping that rule and replacing it with something else. I think the only data matchers left are the ones for the keyvalues. Not sure if you have any ideas on how to address those. Thanks in advance and I do apologize for my shallow knowledge on the matter.

Thanks for your patience and making all those changes! Those grok parser rules are really tricky and it's not obvious that using data to much can have some bad side effects. Keeping the data matchers for the json/kv matching at the end of the rules is fine. What we really try to avoid is having multiple data matchers (or .*) in a single rule.

I'll deploy this integration to our staging env for a final check then approve.

@brunorenier brunorenier added the assets/deploy-logs-staging ONLY USED BY Logs Backend - Validates that a PR is OK to go to staging label Feb 6, 2025
@steveny91 steveny91 added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit 1b353f2 Feb 10, 2025
40 checks passed
@steveny91 steveny91 deleted the sy/keda-logs branch February 10, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets/deploy-logs-staging ONLY USED BY Logs Backend - Validates that a PR is OK to go to staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants