Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement templated topic #43

Merged
merged 1 commit into from
Jun 4, 2020
Merged

Conversation

jbguerraz
Copy link
Contributor

Hello :)

Thank you for such adapter!

We need to distribute the metrics into different topics (one per cluster component), so we implemented templated topic relying on golang template/text (fast - we implemented the same logic for nats-kafka adapter and benchmarked it : nats-io/nats-kafka#11), and on metric labels.

For the sake of clarifying our use case:
Our cluster is made of a set of components (µservices, applications,...). Each component has an associated channel and each component produce logs, metrics, events and traces.
Let's say the component A got the channel component.a, its logs will be published in kafka topic logs.component.a, its metrics will be published in kafka topic metrics.component.a and so on. Then spark applications can subscribe to the data that matter to them. For instance, one spark application may care about the ingress controller metrics, it then gonna subscribes to metrics.component.ingress.
Eventually, one may wish to have a per-metric topic, or even a per component per metric topic (metrics.{{ index . "componentname" }}.{{ index ."__name__" }}).

@palmerabollo
Copy link
Member

Hi @jbguerraz , thanks for your first contribution.

I think this change breaks current unit tests because some interfaces change from [][]byte to map[string][][]byte:

# github.com/Telefonica/prometheus-kafka-adapter
./serializers_test.go:54:36: non-integer slice index i
./serializers_test.go:54:47: cannot convert metric[:] (type [][]byte) to type string
./serializers_test.go:84:36: non-integer slice index i
./serializers_test.go:84:47: cannot convert metric[:] (type [][]byte) to type string
FAIL	github.com/Telefonica/prometheus-kafka-adapter [build failed]

Could you please fix the tests? It's also a good chance to add a new one to cover templated topics.

I've just added #44 to make Travis pass unit tests in the CI pipeline, BTW.

@jbguerraz jbguerraz force-pushed the templated-topic branch 2 times, most recently from 6cb36e7 to 8c801b6 Compare June 1, 2020 16:33
@jbguerraz
Copy link
Contributor Author

Hola @palmerabollo :)
Thank you! and sorry, didn't pay attention to the tests.
I fixed the tests and added a test for templated topic.

@palmerabollo
Copy link
Member

It's our fault, it should be automated in the CI pipeline. It looks good to me. Let's give @jpfe-tid a couple of days to review it and release a new version. Thanks again for your time to improve the project.

@jpfe-tid jpfe-tid self-requested a review June 2, 2020 13:08
Copy link
Contributor

@jpfe-tid jpfe-tid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • add additional checks at substring template func
  • don't take the reference of a loop variable

config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
config.go Show resolved Hide resolved
config.go Show resolved Hide resolved
handlers.go Outdated Show resolved Hide resolved
handlers.go Outdated Show resolved Hide resolved
handlers.go Show resolved Hide resolved
config.go Show resolved Hide resolved
@jbguerraz
Copy link
Contributor Author

Thank you for the review @jpfe-tid !

handlers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jpfe-tid jpfe-tid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your contribution @jbguerraz !

@jpfe-tid jpfe-tid merged commit a58b6d1 into Telefonica:master Jun 4, 2020
Copy link
Contributor

@jpfe-tid jpfe-tid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbguerraz
Copy link
Contributor Author

@jpfe-tid would you mind amending the commit in order to remove my email address please ? :) sorry and thank you!

@jpfe-tid
Copy link
Contributor

jpfe-tid commented Jun 4, 2020

Hi @jbguerraz,

I'm sorry but I can't do that now that the PR has been merged and a release has been published. It would require a force push to the master branch.

Your email address is attached to all your commits (e.g. 3199eb9).

Check this GitHub link for how to setup a privacy email alias: https://help.github.com/en/github/setting-up-and-managing-your-github-user-account/setting-your-commit-email-address#setting-your-commit-email-address-on-github

Check my commits as an example: 0ba161e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants