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.

Sign up for GitHub

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

Jump to bottom

Add execution measurement metrics to `FilterChain` #262

Merged

markmandel merged 1 commit into main from measure

Jun 4, 2021

Collaborator

XAMPPRocky commented May 18, 2021 •

edited

Loading

This PR adds a measure filter that accepts any filter and measure each of its operations. Since this is the first nested filter there were a couple of other changes needed.

ConcatBytes's filter and config are now public. This is in order to construct the filter statically in the tests. I think we should probably have these public for all filters so that they can be easily construct-able statically when using quilkin as a library.
FilterRegistry is now Arc<RwLock<HashMap<K, RwLock<V>> to make it concurrent safe and cheaply clone-able.
quilkin::run now accepts a closure that passes in a FilterRegistry, this is to allow you to construct filters like MeasureFactory which requires access to the FilterRegistry.

resolves #167

XAMPPRocky requested review from markmandel and iffyio

May 18, 2021 06:52

google-cla bot added the cla: yes label

jahrlin reviewed

View reviewed changes

Collaborator

jahrlin left a comment

reviewing only Prom/metric related things, as I don't know Rust yet :)

will the metric names have a prefix? otherwise we should add one, see: https://prometheus.io/docs/practices/naming/#metric-names

src/extensions/filters/measure.rs Outdated Show resolved Hide resolved

src/extensions/filters/measure.rs Outdated Show resolved Hide resolved

src/extensions/filters/measure.rs Outdated Show resolved Hide resolved

Collaborator Author

XAMPPRocky commented May 18, 2021

@jahrlin Thank you for the review! I'm very new to using prometheus so any pointers are appreciated. 😄

XAMPPRocky force-pushed the measure branch 4 times, most recently from 20d10ef to 34fd6bc Compare

May 18, 2021 10:05

XAMPPRocky requested a review from jahrlin

May 18, 2021 10:05

jahrlin approved these changes

View reviewed changes

Collaborator

jahrlin left a comment

metrics part LGTM!

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch 2 times, most recently from 2be1362 to 11976cc Compare

May 19, 2021 06:38

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky mentioned this pull request

Implement Github notification bot. #264

Merged

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch from 11976cc to 57d5476 Compare

May 20, 2021 10:33

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch from 57d5476 to 9a0216b Compare

May 20, 2021 11:20

This comment has been minimized.

Sign in to view

XAMPPRocky mentioned this pull request

review metrics to prevent high-cardinality data #263

Closed

XAMPPRocky force-pushed the measure branch from 9a0216b to 9e2de77 Compare

May 20, 2021 11:43

XAMPPRocky force-pushed the measure branch 2 times, most recently from 2c96087 to aa3de69 Compare

June 3, 2021 11:46

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch from aa3de69 to 6e6b417 Compare

June 3, 2021 11:53

This comment has been minimized.

Sign in to view

iffyio approved these changes

View reviewed changes

Collaborator

iffyio left a comment

LGTM!

XAMPPRocky force-pushed the measure branch 4 times, most recently from 37f9475 to 3cf0d77 Compare

June 4, 2021 06:15

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch from 3cf0d77 to eb7f00a Compare

June 4, 2021 06:19

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

XAMPPRocky force-pushed the measure branch from eb7f00a to 1b33e69 Compare

June 4, 2021 06:23

XAMPPRocky requested a review from markmandel

June 4, 2021 06:24

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

iffyio reviewed

View reviewed changes

Collaborator

iffyio left a comment

left some comments on the filter metrics documentation

docs/extensions/filters/filters.md Outdated Show resolved Hide resolved

docs/extensions/filters/filters.md Outdated Show resolved Hide resolved

docs/extensions/filters/filters.md Outdated Show resolved Hide resolved

XAMPPRocky force-pushed the measure branch from 9f8f0bb to ae42d54 Compare

June 4, 2021 06:45


          Add filter duration metrics to FilterChain

065ebb7

XAMPPRocky force-pushed the measure branch from ae42d54 to 065ebb7 Compare

June 4, 2021 06:45

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Collaborator

quilkin-bot commented Jun 4, 2021

Build Succeeded 🥳

Build Id: b91f91ae-f1f0-416b-855a-4b736bda6612

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/262/head:pr_262 && git checkout pr_262
cargo build

markmandel merged commit b98ac93 into main

markmandel deleted the measure branch

June 4, 2021 20:44

markmandel mentioned this pull request

Metric: Total Packet Processing time #292

Closed

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

Labels