performancecopilot / pcp

Performance Co-Pilot
https://pcp.io
Other
963 stars 235 forks source link

libpcp: support help text for derived metrics #656

Closed natoscott closed 4 years ago

kmcdonell commented 4 years ago

@natoscott is this still a priority? Assuming the answer is "yes" do you have any thoughts on how this should be added syntactically? The current name = expr syntax does not have any obvious hook to latch onto.

natoscott commented 4 years ago

@kmcdonell we'd very much still like to see it, yes. One possible syntax could be:

name = expr name.oneline = "blah blah" name.helptext = "mumble bumble,\nstumble"

... or something like that? That'd allow for derived-metric-specific labels to arrive too at some point.

kmcdonell commented 4 years ago

@natoscott I don't think that will work. "name" could be "foo.bar.online" (a valid metric name!). If you're expecting multiple metric attributes (oneline, helptext, labels, ...), then we need something more general, ... how 'bout name(attr) = "string" where attr is one of a small number of keywords, e.g. oneline, helptext, ... the () could also be [] or {} or a simple delimiter like colon (:)

natoscott commented 4 years ago

@kmcdonell right you are :) ... yep, name(attr) looks good, or colon - either way seems nice and clean to me.

kmcdonell commented 4 years ago

@natoscott OK ... now to the issues 8^)

  1. name=... initial parsing is in pmLoadDerivedConfig() outside the derived metric expression parser, so this is not simple as we can't leverage the lexer for the "string" and we have to parse the name(attr) part outside yacc
  2. does name(attr) have to come after name=expr in a config file (forward attr declarations make it much more complicated)
  3. do we need a variant of pmRegisterDerivedMetric() (and possibly the older pmRegisterDerived()) that does not have an expr argument, but accepts additional int attr and char *string args? pmRegisterDerivedMetricAttrIsAlmostLongEnoughToBeInsane()!
natoscott commented 4 years ago

@natoscott OK ... now to the issues 8^)

:)

1. name=... initial parsing is in pmLoadDerivedConfig() outside the derived metric expression parser, so this is not simple as we can't leverage the lexer for the "string" and we have to parse the name(attr) part outside yacc

Hmm. Ouch.

2. does name(attr) have to come after name=expr in a config file (forward attr declarations make it much more complicated)

I think that's a reasonable assumption / assertion to make.

3. do we need a variant of pmRegisterDerivedMetric() (and possibly the older pmRegisterDerived()) that does not have an expr argument, but accepts additional int attr and char *string args?  pmRegisterDerivedMetricAttrIsAlmostLongEnoughToBeInsane()!

I expect these annotations are always going to come from configuration files only, so we should be able to do away with those IMO.

kmcdonell commented 4 years ago

All done here as of commits 1f73f48 and 197f7e7.