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

graphite dot munging overhaul #1008

Closed
Dieterbe opened this issue Aug 21, 2018 · 7 comments
Closed

graphite dot munging overhaul #1008

Dieterbe opened this issue Aug 21, 2018 · 7 comments
Assignees
Labels

Comments

@Dieterbe
Copy link
Contributor

Dieterbe commented Aug 21, 2018

our carbon fix to eat up a trailing dot (#694) was only the beginning

as it turns out, graphite will:

  • eat up any amount of leading .
  • turn any sequence of . within a metric key into a single .

I did a bunch of testing with both graphite and MT (via fakemetrics/mdm)

case input key whisper graphite MT (current)
1 a.b....c /a/b/c.wsp reduce sequences of multi dots within key to single grafana editor shows * for these positions, requests *. for each extraneous dots and thus ~works
2 ....foo /foo.wsp eat leading dots leads to errors
3 .... not found doesn't work no trace of data. can't be queried
4 a.b. /a/b/.wsp broken in GUI same as case 1

we must handle this properly as well. in particular, metrictank I think should work internally with the "cleaned" up names. because:

  1. it makes the index simpler, if we can assume a standard format (with no leading dot, and no sequences of dots within the name). see crash bug PANIC: runtime error: invalid memory address or nil pointer dereference #811 , it is also practically impossible to do string interning without canonical names.
  2. different inputs that reduce to the same canonical name are the same series in graphite, so they should be as well in metrictank. we generate our id's based off of the name (and others), so the name must be canonical.

So I propose when we convert from carbon to mdm format, we canonical-ize (in carbon-relay-ng),
but for people who don't deploy the new carbon-relay-ng version timely, or who generate their own mdm payloads, we must still accommodate them the same way. and we should also update the MT carbon input.

Will the transition be seamless? Not 100%.. for 2/3 yes, for 1 and 4 i need to look into if anyone is sending (and querying) metrics like this.

TODO:

  • improve carbon20 lib, update in MT to leverage it for its carbon input
  • chomp leading dots in carbon-relay-ng when converting from carbon to mdm. consume leading dots when converting from carbon carbon-relay-ng#296
  • chomp leading dots in MT kafka input
  • disallow keys that are solely made of . in default carbon validation crng, in carbon20 lib (and MT carbon), in MT mdm input, and in tsdbgw validation
@robert-milan
Copy link
Contributor

#1212 implements the EatDots function from raintank/schema#34. So, once it is merged Metrictank will properly handle leading, trailing, and consecutive dots (.).

The other items on the TODO list are still outstanding:

@Dieterbe
Copy link
Contributor Author

sounds good.

Will the transition be seamless? Not 100%.. for 2/3 yes, for 1 and 4 i need to look into if anyone is sending (and querying) metrics like this.

as discussed in meeting, we can forego this. it's unlikely to be an issue for anyone, and if it is, we can help them rectify it.

In the TODO list I think I forgot about tsdbgw. we should also canonicalize in tsdbgw because customers may have other clients than carbon-relay-ng sending to tsdbgw

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Nov 4, 2019

@robert-milan what's the status of this. in particular is #811 (comment) solved?

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jan 2, 2020

I see that crng still has the old code to only eat a single leading dot, not multiple.

@stale
Copy link

stale bot commented Apr 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 4, 2020
@Dieterbe
Copy link
Contributor Author

@robert-milan did you end up fixing this?

@stale stale bot removed the stale label Apr 10, 2020
@stale
Copy link

stale bot commented Jul 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 9, 2020
@stale stale bot closed this as completed Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants