Adopt statsd_exporter mapper #52

travisgroth · 2018-08-11T16:39:59Z

This is to address #37. Also would implicitly address #38, #40, #10, and #32 (I think?). There may be additional resolutions from #37 as well.

NB: I actually copied in and modified the statsd mapper and dependencies, as it sits in their main package and thus isn't importable. Tests not updated yet, though basic functionality seems fine in both glob and regex modes. The mapper interface is similar, so the changes in main.go were trivial.

Would like to take the project's temperature on approach/changes and would be happy to add in anything trivial while I have my attention on this.

matthiasr · 2018-08-12T07:39:14Z

Thank you! I'm really happy to see that I'm not the only one who thinks this is a good idea. However, one goal I had was to benefit from improvements to the mapper in statsd_exporter, which will be difficult if we have a modified copy there. I think the first step will need to be to move the mapper into its own package over there; I started to look at that this weekend but I'm not sure how far I actually got. I have a train ride coming up and will try to sort this out then, but no promises. if you want to give the packagisation a go shout!

…

On Sat, Aug 11, 2018, 18:40 Travis Groth ***@***.***> wrote: @matthiasr <https://github.com/matthiasr> This is to address #37 <#37>. Also would implicitly address #38 <#38>, #40 <#40>, #10 <#10>, and #32 <#32> (I think?). There may be additional resolutions from #37 <#37> as well. NB: I actually copied in and modified the statsd mapper and dependencies, as it sits in their main package and thus isn't importable. Tests not updated yet, though basic functionality seems fine in both glob and regex modes. The mapper interface is similar, so the changes in main.go were trivial. Would like to take the project's temperature on approach/changes and would be happy to add in anything trivial while I have my attention on this. ------------------------------ You can view, comment on, or merge this pull request online at: #52 Commit Summary - Pull in YAML based mapper from statsd exporter - Remove statsd names File Changes - *A* action.go <https://github.com/prometheus/graphite_exporter/pull/52/files#diff-0> (42) - *M* main.go <https://github.com/prometheus/graphite_exporter/pull/52/files#diff-1> (4) - *M* mapper.go <https://github.com/prometheus/graphite_exporter/pull/52/files#diff-2> (166) - *A* match.go <https://github.com/prometheus/graphite_exporter/pull/52/files#diff-3> (41) Patch Links: - https://github.com/prometheus/graphite_exporter/pull/52.patch - https://github.com/prometheus/graphite_exporter/pull/52.diff — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICBmc7rTrwYqOwPWD61iQ0Nbeoq6aAks5uPwjggaJpZM4V5Oqn> .

This allows the mapping logic and configuration format to be re-used by the graphite exporter. Enables prometheus/graphite_exporter#37, cf. prometheus/graphite_exporter#52 (comment) Signed-off-by: Matthias Rampke <[email protected]> add missing license header Signed-off-by: Matthias Rampke <[email protected]>

This allows the mapping logic and configuration format to be re-used by the graphite exporter. Enables prometheus/graphite_exporter#37, cf. prometheus/graphite_exporter#52 (comment) Signed-off-by: Matthias Rampke <[email protected]>

matthiasr · 2018-08-12T11:15:11Z

@travisgroth please take a look: prometheus/statsd_exporter#140

travisgroth · 2018-08-12T16:48:57Z

I'm not sure how valuable directly importing that mapper is but it would certainly be better if it was possible. To be honest, it might make sense to eventually consolidate the projects, or shift more to a mostly shared code base. Anyway, I'm on board with small steps here.

Can I propose continuing on this PR in parallel while taking care to honor the existing statsd_exporter mapper interface? I made no changes to the original code other than some trivial renaming. I'm open to depending on a (your?) fork instead, since that is now an option.

If statsd doesn't want to expose the mapper or the PR sits for a while, I don't want to have this change blocked, as I'd like to get the regex support in the near term. Once the statsd_exporter mapper PR is resolved we can trivially switch to directly consume it, if appropriate.

I can probably pick this up tomorrow evening if that all sounds good.

matthiasr · 2018-08-13T07:26:55Z

Could you review my mapper packaging PR? I'm maintainer for both projects so I can do whatever is necessary to unblock doing this right. I agree that a mostly shared codebase is the goal. The next candidate after the mapper is the metrics lookup/storage/collection code, which needs work anyway.

…

On Sun, Aug 12, 2018, 18:48 Travis Groth ***@***.***> wrote: I'm not sure how valuable directly importing that mapper is but it would certainly be better if it was possible. To be honest, it might make sense to eventually consolidate the projects, or shift more to a mostly shared code base. Anyway, I'm on board with small steps here. Can I propose continuing on this PR in parallel while taking care to honor the existing statsd_exporter mapper interface? I made no changes to the original code other than some trivial renaming. I'm open to depending on a (your?) fork instead, since that is now an option. If statsd doesn't want to expose the mapper or the PR sits for a while, I don't want to have this change blocked, as I'd like to get the regex support in the near term. Once the statsd_exporter mapper PR is resolved we can trivially switch to directly consume it, if appropriate. I can probably pick this up tomorrow evening if that all sounds good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICBiHeEXO_PJBtw-648QN7LuadH7U2ks5uQFx5gaJpZM4V5Oqn> .

travisgroth · 2018-08-14T00:37:16Z

I'm maintainer for both projects
so I can do whatever is necessary to unblock doing this right.

Ah, did not realize. Great! Will check out the PR.

travisgroth · 2018-08-15T05:23:43Z

Basic refactor done but I vendored your branch for the moment.

Would like to at least try to add a little testing around the processLine() method before calling this done. Should be able to do so later this week.

matthiasr

Looks good so far! I'm always in favor of tests. Can you also update the README and prepare a note about the breaking change in the changelog?

Does @bakins' config converter work for this?

matthiasr · 2018-08-15T06:22:16Z

main.go

 	if present {
-		name = labels["name"]
+		name = invalidMetricChars.ReplaceAllString(mapping.Name, "_")


wouldit make sense to add a helper for this to the mapper package?

I refactored that bit anyway (not pushed yet) and, as it turns out, it seems like something later in the data path cleans that up. I figured that out working on the unit test. So, there may already be a helper somewhere in the prom code that can be leveraged, but I haven't gone looking just yet.

travisgroth · 2018-08-19T15:18:07Z

@matthiasr OK I think I'm done making code changes here unless you see something off. Please take a look over as I did have to make some updates in main to support testing.

Will work on readme/changelog.

Re: converter - you mean this, right? https://github.com/bakins/statsd-exporter-convert I'm not familiar with project history here but offhand it does look that way. Did they originally share that same parser? I will give it a cursory try and link over in the docs if it appears to work.

matthiasr · 2018-08-20T07:59:55Z

Yes, historically the mappers were copies, but they diverged.

matthiasr

Looks good so far! Could you take a look at the build failures?

matthiasr · 2018-08-20T08:05:24Z

vendor/vendor.json

@@ -105,5 +117,5 @@
 			"revisionTime": "2017-07-27T04:22:29Z"
 		}
 	],
-	"rootPath": "github.com/prometheus/graphite_exporter"
+	"rootPath": "github.com/travisgroth/graphite_exporter"


We'll need to change this back before it hits master … I think one way to be able to develop locally under a different name is to check out the exporter at github.com/prometheus/graphite_exporter and add your fork as an additional remote.

I probably broke the vendor.json rootPath early on so let me fix that and repath.

Edit - nevermind. I was conflating a local error with the CI error. Looks like just me fumbling with govendor.

matthiasr · 2018-08-20T08:13:35Z

README.md

+```
+mappings:
+- match: test.dispatcher.*.*.*
+name: dispatcher_events_total


I think the indentation here is off?

matthiasr · 2018-08-21T08:38:04Z

I don't understand the last commit (247f94c) … IIRC as it stands the client library will refuse to serve these anyway. OTOH that's probably something we need to address (outside of this PR) so it's good to be able to parse them. Ok carry on 🦆

Please keep in mind that you need to put the sign-off on every commit before I can merge this.

travisgroth · 2018-08-21T11:44:05Z

On Aug 21, 2018, at 4:38 AM, Matthias Rampke ***@***.***> wrote: I don't understand the last commit (247f94c <247f94c>) … IIRC as it stands the client library will refuse to serve these anyway. OTOH that's probably something we need to address (outside of this PR) so it's good to be able to parse them. Ok carry on 🦆

This was to fix a bug I introduced. When you use the Name as the key on c.samples, you'll lose all label based differentiation. Eg, [ my.metric.foo and my.metric.bar -> my_metric{job="foo"} and my_metric{job="bar"} ] would wind up overwriting each other with the last one being the only metric recorded in the map. By using the original graphite metric path you preserve the uniqueness so that each is stored separately. The alternative and slightly more correct approach is to derive the key from the mapped name and labels but this was the original implementation and it is functional.

Please keep in mind that you need to put the sign-off on every commit before I can merge this.

I will rebase this into something more reasonable.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#52 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6VIgXFP0LbpBMaaTkxByPM-yIK-5jLks5uS8bxgaJpZM4V5Oqn>.

matthiasr

Ah that makes sense then! This looks good to me, is there anything else you want to do besides rebasing?

matthiasr · 2018-08-21T15:54:59Z

README.md

+graphite metrics into labeled Prometheus metrics via YAML configuration file.  This file shares syntax and logic with [statsd_exporter](https://github.com/prometheus/statsd_exporter).  Please follow the statsd_exporter documentation for usage information.  However, graphite_exporter does not support *all* parsing features at this time.  Specific differences in functionality:
+
+- timer_type is not supported.  All metrics are assumed to be gauges as graphite does not distinguish.
+- quantiles are not supported


isn't this implied by the first statement? what about histograms?

Hmm I suppose that is all implied by just saying "only gauges" right now. Good point.

travisgroth · 2018-08-22T00:56:40Z

I think that's all the changes for me. Will try to put the rebase together this evening.

Signed-off-by: Travis Groth <[email protected]>

matthiasr · 2018-08-22T07:24:18Z

Looks great, thank you for bearing with me and getting this through!

travisgroth · 2018-08-22T12:17:06Z

Not a problem. Glad to get this in!

matthiasr mentioned this pull request Aug 12, 2018

Move mapper to a package prometheus/statsd_exporter#140

Merged

matthiasr reviewed Aug 15, 2018

View reviewed changes

travisgroth force-pushed the statsd_mapper branch from 6870aa4 to 2dee093 Compare August 19, 2018 16:12

matthiasr reviewed Aug 20, 2018

View reviewed changes

travisgroth force-pushed the statsd_mapper branch from b017561 to ad00524 Compare August 20, 2018 12:20

matthiasr reviewed Aug 21, 2018

View reviewed changes

travisgroth force-pushed the statsd_mapper branch from 247f94c to 45a2c44 Compare August 22, 2018 03:11

travisgroth added 4 commits August 21, 2018 23:15

Switch to statsd_exporter/pkg/mapper

c883c42

Signed-off-by: Travis Groth <[email protected]>

Add testing and address any issues

54248d9

Signed-off-by: Travis Groth <[email protected]>

Update CI/build details

d1e836a

Signed-off-by: Travis Groth <[email protected]>

Update README for config syntax changes

5d626ca

Signed-off-by: Travis Groth <[email protected]>

travisgroth force-pushed the statsd_mapper branch from 45a2c44 to 5d626ca Compare August 22, 2018 03:18

matthiasr merged commit 19f9a6f into prometheus:master Aug 22, 2018

This was referenced Aug 22, 2018

Support comments #51

Closed

Config supports metric comments #8

Closed

name label clashes with series name #39

Closed

This was referenced Aug 22, 2018

Use metrics mapping from statsd_exporter #37

Closed

Use YAML configuration format #38

Closed

efficks mentioned this pull request Feb 11, 2019

Accept 'name' as a label #75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt statsd_exporter mapper #52

Adopt statsd_exporter mapper #52

travisgroth commented Aug 11, 2018

matthiasr commented Aug 12, 2018 via email

matthiasr commented Aug 12, 2018

travisgroth commented Aug 12, 2018

matthiasr commented Aug 13, 2018 via email

travisgroth commented Aug 14, 2018

travisgroth commented Aug 15, 2018

matthiasr left a comment

matthiasr Aug 15, 2018

travisgroth Aug 17, 2018

travisgroth commented Aug 19, 2018

matthiasr commented Aug 20, 2018

matthiasr left a comment

matthiasr Aug 20, 2018

travisgroth Aug 20, 2018 •

edited

Loading

matthiasr Aug 20, 2018

matthiasr commented Aug 21, 2018

travisgroth commented Aug 21, 2018 via email

matthiasr left a comment

matthiasr Aug 21, 2018

travisgroth Aug 22, 2018

travisgroth commented Aug 22, 2018

matthiasr commented Aug 22, 2018

travisgroth commented Aug 22, 2018

Adopt statsd_exporter mapper #52

Adopt statsd_exporter mapper #52

Conversation

travisgroth commented Aug 11, 2018

matthiasr commented Aug 12, 2018 via email

matthiasr commented Aug 12, 2018

travisgroth commented Aug 12, 2018

matthiasr commented Aug 13, 2018 via email

travisgroth commented Aug 14, 2018

travisgroth commented Aug 15, 2018

matthiasr left a comment

Choose a reason for hiding this comment

matthiasr Aug 15, 2018

Choose a reason for hiding this comment

travisgroth Aug 17, 2018

Choose a reason for hiding this comment

travisgroth commented Aug 19, 2018

matthiasr commented Aug 20, 2018

matthiasr left a comment

Choose a reason for hiding this comment

matthiasr Aug 20, 2018

Choose a reason for hiding this comment

travisgroth Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

matthiasr Aug 20, 2018

Choose a reason for hiding this comment

matthiasr commented Aug 21, 2018

travisgroth commented Aug 21, 2018 via email

matthiasr left a comment

Choose a reason for hiding this comment

matthiasr Aug 21, 2018

Choose a reason for hiding this comment

travisgroth Aug 22, 2018

Choose a reason for hiding this comment

travisgroth commented Aug 22, 2018

matthiasr commented Aug 22, 2018

travisgroth commented Aug 22, 2018

travisgroth Aug 20, 2018 •

edited

Loading