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

Provide option to collect custom metrics inside filters #384

Merged
merged 5 commits into from
Sep 4, 2017

Conversation

chandu188
Copy link
Contributor

@chandu188 chandu188 commented Aug 1, 2017

Now one can collect the custom metrics inside the filter using the methods defined in the filters.Metrics interface.
Mentioned in this issue #352
Please give your suggestions.

Following is the idea the PR implemented:

  1. Add an interface 'Metrics' in the filters package with following methods
    - MeasureSince(key, time)
    - IncCounter(key)
  2. Add a method in the FilterContext interface to expose the above interface
  3. add an extra field (metrics) in the context and implement the Metrics method added in the FilterContext interface
  4. Implement the Metrics interface in metrics.Metrics struct.

@aryszka
Copy link
Contributor

aryszka commented Aug 2, 2017

👍

@aryszka
Copy link
Contributor

aryszka commented Aug 2, 2017

@lmineiro what do you think about this extension?

@lmineiro
Copy link
Contributor

lmineiro commented Aug 2, 2017

This looks like one good start but I wouldn't leave it as it is.

Although the metrics registry seems to be properly protected, the namespacing mentioned before is not. This can, potentially, lead to:

  • A filter can use the same metric key as any other filter (breaking metrics for both)
  • A filter can use a metric key that belongs to Skipper itself

One simple way could be to always prefix a filter's key with the filter name, which already needs to be unique in the filter registry.

What do you think?

@aryszka
Copy link
Contributor

aryszka commented Aug 2, 2017

prefixing is a good point, i agree. It can be done in the proxy where it is iterating over the filters.


type FilterMetrics struct {
Metrics *metrics.Metrics
FilterName string
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer not using this as a package visible field. Let me contemplate a bit on a different approach..

Copy link
Contributor Author

@chandu188 chandu188 Aug 4, 2017

Choose a reason for hiding this comment

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

you are right. I should have thought a bit more before pushing the code. With this code, I can easily change the FiilterName to whatever in the filters which we don't want to.

@aryszka
Copy link
Contributor

aryszka commented Aug 9, 2017

@lmineiro we applied the prefixing. Could you please check if it looks ok like this?

@aryszka
Copy link
Contributor

aryszka commented Sep 4, 2017

👍

1 similar comment
@vigneshshanmugam
Copy link

👍

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.

4 participants