performancecopilot / speed

A Go implementation of the PCP instrumentation API
MIT License
37 stars 6 forks source link

Client name with invalid characters does not work #53

Open lzap opened 6 years ago

lzap commented 6 years ago

It does not appear in PCP, I was tracking it down for an hour until I found that "client-123" won't work. Just a reminder for googlers.

suyash commented 6 years ago

@lzap this is weird, the client name is used in two places, calculating the path for the mmv file and calculating the hash for the cluster ID. Looking into it.

suyash commented 6 years ago

@natoscott I tried https://github.com/performancecopilot/speed/blob/master/examples/basic_histogram/main.go, but renamed the client from histogram_test to histogram_test-1. According to mmvdump everything seems fine (it generates the cluster ID as 2060). However, on my installation also, metrics do not seem to be visible inside PCP.

lzap commented 6 years ago

Yeah, everything seems to be okay (no errors), except nothing appears via pminfo. I was quite confused and lost some time debugging this until I realized the dash as the root cause.

natoscott commented 6 years ago

@lzap "client-123" is not a valid metric name component by the rules in PMNS(5) ...

   Each component in the pathname must begin with an alphabetic character,
   and  be followed by zero or more characters drawn from the alphabetics,
   the digits and the underscore ``_'') character.  For alphabetic charac‐
   ters in a pathname component, upper and lower case are distinguished.

... so dropping such a name is correct, but probably a warning should be issued? Oh, it may actually be the MMV PMDA itself that is dropping this metric actually - either way, better to pick valid PCP metric names based on the above man page.

suyash commented 6 years ago

"client-123" is not a valid metric name

@natoscott just to be sure, will this rule apply to client name also, again, the client name is a speed specific feature used for two purposes

natoscott commented 6 years ago

@suyash yep, the file name will be used as part of the metric name by default (mmv..*) - so should also have these name rules applied.