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 a threshold for search if want to emit a metric #165

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

Aiqin-Aiven
Copy link
Contributor

@Aiqin-Aiven Aiqin-Aiven commented Feb 2, 2024

in case some bad regex search defined,
it would be easier to identify the offender

The threshold is defined as threshold_for_metric_emit with default value 10 seconds.

The metric would look like this:
image
with regex tag on it.

@Aiqin-Aiven Aiqin-Aiven marked this pull request as draft February 2, 2024 10:03
@mhoffm-aiven
Copy link
Contributor

What if the search never finishes though? I think this is better served by converting to using re2!

@Aiqin-Aiven Aiqin-Aiven force-pushed the aiqin-search-metric branch from 95362f1 to 695f21b Compare March 11, 2024 00:35
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 63.32%. Comparing base (390e69b) to head (e06ad2f).
Report is 6 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   63.13%   63.32%   +0.19%     
==========================================
  Files          19       19              
  Lines        2083     2094      +11     
==========================================
+ Hits         1315     1326      +11     
  Misses        768      768              
Files Coverage Δ
journalpump/journalpump.py 64.00% <66.66%> (+0.97%) ⬆️

... and 3 files with indirect coverage changes

@Aiqin-Aiven Aiqin-Aiven force-pushed the aiqin-search-metric branch from 695f21b to c39c401 Compare March 11, 2024 05:35
@Aiqin-Aiven
Copy link
Contributor Author

ishes though? I think this is better served by converting to using re2!
We had a discussion in the team and journalpump is going to be replaced by vector, so we stick to the plan to get some metrics out.

@Aiqin-Aiven Aiqin-Aiven marked this pull request as ready for review March 11, 2024 05:43
Copy link
Contributor

@allhailwesttexas allhailwesttexas left a comment

Choose a reason for hiding this comment

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

Some small comments. Also, is there any test coverage for this change?

journalpump/journalpump.py Outdated Show resolved Hide resolved
journalpump/journalpump.py Outdated Show resolved Hide resolved
@Aiqin-Aiven Aiqin-Aiven force-pushed the aiqin-search-metric branch from 7d0e907 to 18b6bac Compare March 27, 2024 03:15
Copy link
Contributor

@RommelLayco RommelLayco left a comment

Choose a reason for hiding this comment

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

i don't see the test you added

journalpump/journalpump.py Outdated Show resolved Hide resolved
@Aiqin-Aiven Aiqin-Aiven force-pushed the aiqin-search-metric branch from 18b6bac to 50e228e Compare March 28, 2024 00:05
@Aiqin-Aiven
Copy link
Contributor Author

i don't see the test you added

hmm, it was there at commit: 7d0e907, I did something wrong yesterday probably

in case some bad regex search defined,
it would be easier to identify the offender
@Aiqin-Aiven Aiqin-Aiven force-pushed the aiqin-search-metric branch from d65493b to e06ad2f Compare March 28, 2024 01:59
Copy link
Contributor

@RommelLayco RommelLayco left a comment

Choose a reason for hiding this comment

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

lgtm

@RommelLayco RommelLayco merged commit a71f5aa into master Mar 28, 2024
6 checks passed
@RommelLayco RommelLayco deleted the aiqin-search-metric branch March 28, 2024 02:09
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.

5 participants