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 more metrics for extractors #3332

Merged
merged 2 commits into from Jan 11, 2017
Merged

Add more metrics for extractors #3332

merged 2 commits into from Jan 11, 2017

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Jan 10, 2017

This change adds the following metrics for extractors:

  • conditionHits (number of times the condition succeeded)
  • conditionMisses (number of times the condition failed)
  • conditionTime (time spent in evaluating the condition)
  • completeExecutionTime (total time spent in the extractor)

Fixes #2959

This change adds the following metrics for extractors:

* Added conditionHits (number of times the condition succeeded)
* Added conditionMisses (number of times the condtion failed)
* Added conditionTime (time spent in evaluating the condition)
* completeExecutionTime (total time spent in the extractor)

Fixes #2959
@joschi joschi added this to the 2.2.0 milestone Jan 10, 2017
@joschi
Copy link
Contributor Author

@joschi joschi commented Jan 10, 2017

The diff is best viewed with whitespace ignored: https://github.com/Graylog2/graylog2-server/pull/3332/files?w=1

@kroepke kroepke self-requested a review Jan 11, 2017
Copy link
Member

@kroepke kroepke left a comment

Other than these performance/cosmetic questions, lgtm!

this.conditionTimerName = name(metricsPrefix, "conditionTime");
this.executionTimerName = name(metricsPrefix, "executionTime");
this.converterTimerName = name(metricsPrefix, "converterExecutionTime");
this.completeTimerName = name(metricsPrefix, "completeExecutionTime");

This comment has been minimized.

@kroepke

kroepke Jan 11, 2017
Member

Can we also pre-create these timers like the counters below?
That should save another hash lookup and instanceOf check in metricRegister.timer, right?

This comment has been minimized.

@joschi

joschi Jan 11, 2017
Author Contributor


final Result[] results = run(field);
final List<Result> reverseList = Arrays.stream(results)

This comment has been minimized.


final Timer.Context timerContext = metricRegistry.timer(getTotalTimerName()).time();
try (final Timer.Context executionTimer = metricRegistry.timer(getExecutionTimerName()).time()) {

This comment has been minimized.

@kroepke

kroepke Jan 11, 2017
Member

nitpick: I prefer to either name these ignored or annotate them to avoid the IDE warnings (also elsewhere in this file)

This comment has been minimized.

@joschi

joschi Jan 11, 2017
Author Contributor

Copy link
Member

@kroepke kroepke left a comment

lgtm, just waiting for builds to come in

@kroepke kroepke merged commit 7d88616 into master Jan 11, 2017
4 checks passed
4 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1262 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@kroepke kroepke deleted the issue-2959 branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants