Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

some tags should not contribute to the metric ID #30

Closed
Dieterbe opened this issue Sep 29, 2015 · 4 comments
Closed

some tags should not contribute to the metric ID #30

Dieterbe opened this issue Sep 29, 2015 · 4 comments

Comments

@Dieterbe
Copy link
Contributor

(*schema.MetricData)(0xc8201de300)({
 OrgId: (int) 1,
 Name: (string) (len=37) "litmus.localhost.dev1.dns.error_state",
 Metric: (string) (len=22) "litmus.dns.error_state",
 Interval: (int) 10,
 Value: (float64) 0,
 Unit: (string) (len=5) "state",
 Time: (int64) 1443523161,
 TargetType: (string) (len=5) "gauge",
 Tags: ([]string) (len=3 cap=3) {
  (string) (len=13) "endpoint_id:1",
  (string) (len=12) "monitor_id:4",
  (string) (len=14) "collector:dev1"
 }
})

Id() uses all tags.
if we get a metric with the exact same name/metric/OrgId and all tags, except monitor_id is now 5, then it should be the same metric with the same ID.

metrics2.0 address such scenarios by having regular tags and meta tags which can change and don't contribute to the ID. see http://metrics20.org/spec/
why are storing monitor_id in there anyway?

as we grow to support all kinds of metrics, it 'll be even more common to have meta tags that should be able to change without changing the ID, so I think we should implement meta tags like in metrics2.0.

@Dieterbe
Copy link
Contributor Author

In addition, I think we should formalize the MetricData format.
in particular:

  • what's the purpose of Name?
  • what's the purpose of "Metric" ?
  • is Name basically Metric, but with tags added into the string? (not really, see monitor_id tag that is not represented into the Name)
  • since Id() already compiles all tags into the metric ID, why does it still use Name, that seems redundant. it could use the Metric which doesn't have tag info in it?
  • why does Id need the tags anyway, since the relevant tags are already included in Name, seems like we can just as well use OrgId + Name and don't bother including the tags?

@Dieterbe
Copy link
Contributor Author

since we want to adopt metrics2.0, we could also just move org-id, unit and target_type into the tags array. i know we want to implement some sort of validation to make sure those values are actually correctly specified, but we can do that on ingest. or actually, since unit and target_type are mandatory, they could stay, but we could move org-id into the tags. I would like to make all tools in this repo general purpose, not tied to litmus. and i don't think not much needs to happen for that (but making org-id optional would be one thing)

@woodsaj
Copy link
Member

woodsaj commented Sep 29, 2015

  • what's the purpose of Name?

Graphite format

  • what's the purpose of "Metric" ?

Kairosdb metric format

  • is Name basically Metric, but with tags added into the string? (not really, see monitor_id tag that is not represented into the Name)

see 1 and 2 above. It is entirely up to the user sending the metric. metric doesnt need to be a subset of name.

  • since Id() already compiles all tags into the metric ID, why does it still use Name, that seems redundant. it could use the Metric which doesn't have tag info in it?

Agreed, we should change this.

  • why does Id need the tags anyway, since the relevant tags are already included in Name, seems like we can just as well use OrgId + Name and don't bother including the tags?

The entire purpose of the Id is to future proof the schema and allow us to transition away from graphite style naming towards more modern metric+tag naming.

(but making org-id optional would be one thing)

OrgId is mandatory. Everything we build needs to be multi Tenant capable. It is trivial to use a multi-tenant product in a single tenant environment, but you cant do the reverse.

@Dieterbe
Copy link
Contributor Author

see #31 and #199

Dieterbe added a commit to raintank/schema that referenced this issue Jul 19, 2016
* rename target_type to mtype - see metrics20/spec@2fa3265
* include mtype and unit into metric id. this is an overdue correction
* base id off of metric, not name. see grafana/metrictank#30 (comment)
* include interval into metric id. e.g. treat it like any other tag.
  after some discussion we think this will be the cleanest way to support multiple intervals for a given metric,
  metrictank just has to merge multiple metrics together. see grafana/metrictank#80
* remove references to nodejs

note: this makes SetId about 27% slower
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants