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

Modify metrics so that names are normalized to legal chars #771

Merged
merged 27 commits into from Sep 20, 2016

Conversation

Projects
None yet
2 participants
@theshadow
Copy link
Contributor

theshadow commented Sep 7, 2016

  • Add function for normalizing metric keys
  • Add tests for metrics functions.
metrics.go Outdated
// This function is used to make metric names safe for all metric services. Specifically, prometheus does
// not support * or / in metric names.
func normalizeKeys(key []string) {
if key != nil {

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

this check is not needed (you can range over nil)

metrics.go Outdated
metriks Metrics

// used for normalizing names by matching '*' and '/' so they can be replaced.
invalidCharactersRE = regexp.MustCompile(`[\*/]`)

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

should the regexp be the same as the one in the prometheus package? the test should be whether the character does not match the regexp (white list instead of black list).

metrics.go Outdated
invalidCharactersRE = regexp.MustCompile(`[\*/]`)
)

// default metrics interface

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

Maybe the comment should be:

Metrics is the interface used by goa to send metrics data. It is compatible with the Metrics struct from the github.com/armon/go-metrics package.
metrics.go Outdated
// configuration and metrics sink
func NewMetrics(conf *metrics.Config, sink metrics.MetricSink) (err error) {
metriks, err = metrics.NewGlobal(conf, sink)
return
}

// SetMetrics initializes goa's metrics instance with the supplied

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

... with the supplied metrics adapter interface.

Xander Guzman
k = strings.Replace(k, allMatcher, allReplacement, -1)

// now replace all other invalid characters with a safe one.
key[i] = invalidCharactersRE.ReplaceAllString(k, normalizedToken)

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

See comment on the regexp, this should be a test to see whether the character is in a set of white listed characters.

@@ -0,0 +1,120 @@
package goa_test

This comment has been minimized.

@raphael

raphael Sep 7, 2016

Member

Tests look great!

Xander Guzman added some commits Sep 7, 2016

Xander Guzman
Xander Guzman
Xander Guzman
Xander Guzman
Xander Guzman
Xander Guzman
Xander Guzman
metrics.go Outdated
}

// SetMetrics initializes goa's metrics instance with the supplied metrics adapter interface.
func SetMetrics(m Metrics) {
metriks = m
mu.Lock()

This comment has been minimized.

@raphael

raphael Sep 19, 2016

Member

The mutex is not needed anymore with atomic.Value

Xander Guzman added some commits Sep 19, 2016

@raphael
Copy link
Member

raphael left a comment

Thanks for the PR! Left a few comments to address, mainly removing the mutex and changing the regexp used by the test code.

metrics.go Outdated
// metriks atomic value storage
metriks atomic.Value

// mu mutex for metriks

This comment has been minimized.

@raphael

raphael Sep 19, 2016

Member

Please remove the mutex, it is not needed anymore (see the first example of sync.Value).

metrics.go Outdated
invalidCharactersRE = regexp.MustCompile(`[\*/]`)

// Taken from https://github.com/prometheus/client_golang/blob/66058aac3a83021948e5fb12f1f408ff556b9037/prometheus/desc.go
metricsNameRE = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_:]*$`)

This comment has been minimized.

@raphael

raphael Sep 19, 2016

Member

The regexp above doesn't seem to be used anywhere. It seems to be the better one though since it is more precise. Could you adjust the code to use this regexp instead of invalidCharactersRE ? It's the reverse condition though so the code needs to be adapted as well...

This comment has been minimized.

@theshadow

theshadow Sep 19, 2016

Contributor

I'm confused, it is actually used in normalize. How were you expecting the normalize function to operate? Currently this is its flow.

normalizeKeys(keys):
    for each key in keys:
         if not metricsNameRE.Match(key):
               key = invalidCharactersRE.Replace(key, "_")

This comment has been minimized.

@raphael

raphael Sep 19, 2016

Member

oh whoops missed that. It seems the test for invalid characters is not sufficient though? there are many other characters than * and / that don't pass the metricsNameRE regexp.

Xander Guzman added some commits Sep 19, 2016

Xander Guzman added some commits Sep 20, 2016

Xander Guzman
Correct metrics errors
* Refactor NoopSink to be NoOpSink
* Create NoOpSink factory method
Xander Guzman
Xander Guzman
Xander Guzman
@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 20, 2016

Thank you for all the work!

@raphael raphael merged commit bc7bea7 into goadesign:master Sep 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

ikawaha added a commit to ikawaha/goa that referenced this pull request Dec 13, 2016

Modify metrics so that names are normalized to legal chars (goadesign…
…#771)

* Add normalizeKeys() function for normalizing metric key names

* Properly synchronize updates to metrics object

@ikawaha ikawaha referenced this pull request Dec 13, 2016

Merged

Backporting unmerged features/fixes to v1 branch. #965

5 of 5 tasks complete

raphael added a commit that referenced this pull request Dec 14, 2016

Backporting unmerged features/fixes to v1 branch. (#965)
* Added the ability to dynamically add/remove keys from the jwt security middleware after creation. (#818)

* Refactor JWT key resolver (#832)

To introduce new "simple" resolver.

* Add an interface instead of using testing.T (#733)

* removed unneeded check which would make an unsafe concurrent modification (#736)

- logit() doesn't need to modify a.keyvals because it was already fixed up in New()

* Modify metrics so that names are normalized to legal chars (#771)

* Add normalizeKeys() function for normalizing metric key names

* Properly synchronize updates to metrics object

* Fix setting a default value to datetime

* Add tests for a default value of datetime

* address issue #738 by creating a title for the test file

* Fix a finalize test

* Revert "Refactor JWT key resolver (#832)"

This reverts commit f6ecb66.

* Revert "Added the ability to dynamically add/remove keys from the jwt security middleware after creation. (#818)"

This reverts commit 4c7fc9d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment