refactor promtail converter sd configs so they reuse prometheus conve… #5069

erikbaranowski · 2023-09-01T19:53:19Z

…rter functionality

PR Description

Use higher level functions from the prometheusconvert API for generating the service discovery river in the promtail converter. Some future work may be done to improve consuming the prometheusconvert API but this gets it all wired up in one place and saves us from modifying the promtail converter when implementing new flow discovery components.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

…rter functionality Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

…vert package Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

erikbaranowski · 2023-09-01T20:13:22Z

converter/internal/prometheusconvert/azure.go

@@ -13,14 +13,14 @@ import (
 )

 func appendDiscoveryAzure(pb *prometheusBlocks, label string, sdConfig *prom_azure.SDConfig) discovery.Exports {
-	discoveryAzureArgs := ToDiscoveryAzure(sdConfig)


these lower level functions do not need be public anymore. we can bring this back later if we need to.

erikbaranowski · 2023-09-01T20:14:00Z

converter/internal/prometheusconvert/prometheusconvert.go

@@ -154,7 +154,7 @@ func appendServiceDiscoveryConfigs(pb *prometheusBlocks, serviceDiscoveryConfig
 			exports = appendDiscoveryLightsail(pb, common.LabelWithIndex(labelCounts["lightsail"]-1, label), sdc)
 		}

-		targets = append(exports.Targets, targets...)
+		targets = append(targets, exports.Targets...)


This fixes a backwards ordering that was not a functional problem but weird. Discovered when leveraging this for promtail.

erikbaranowski · 2023-09-01T20:15:15Z

converter/internal/promtailconvert/internal/build/scrape_builder.go

@@ -53,27 +53,6 @@ func NewScrapeConfigBuilder(
 	}
 }

-func (s *ScrapeConfigBuilder) Validate() {


this is covered by the prometheus service discovery validation function and catches all unsupported sd configs not just explicitly coded ones.

erikbaranowski · 2023-09-01T20:15:54Z

converter/internal/promtailconvert/internal/build/service_discovery.go

+	pb.AppendToFile(s.f)
+
+	for _, target := range targets {
+		s.allTargetsExps = append(s.allTargetsExps, target["__expr__"])


maybe promtail should be using the targets in the same way as prometheus but I'm not going to tackle that in this PR.

It could, but at the same time promtail has other types that are handled as strings, so it would make it more consistent with prometheus, but less consistent with itself.

I generally prefer the method with strings rather than having a special __expr__ label or mandating the type to be an interface so that it can implement Tokenizer. Some export types like type Rules []*Config can not implement Tokenizer (without a bigger refactor) so we can only do it with strings right now. The downside of strings is lack of type safety, but tests cover some of this.

But it's a more general comment, I'm happy with the implementation in this PR.

erikbaranowski · 2023-09-01T20:16:31Z

converter/internal/promtailconvert/internal/build/service_discovery.go

+	s.diags.AddAll(prometheusconvert.ValidateServiceDiscoveryConfigs(sdConfigs))
+}
+
+func toDiscoveryConfig(cfg *scrapeconfig.Config) prom_discover.Configs {


let's turn the promtail SD configs into a type compatible with the current prometheus API

converter/internal/promtailconvert/promtailconvert.go

converter/internal/promtailconvert/internal/build/consul_agent.go

thampiotr

LGTM! Thanks for this, I'm very happy to see the boilerplate get lighter :)

thampiotr · 2023-09-05T08:26:18Z

converter/internal/promtailconvert/internal/build/service_discovery.go

+	pb.AppendToFile(s.f)
+
+	for _, target := range targets {
+		s.allTargetsExps = append(s.allTargetsExps, target["__expr__"])


It could, but at the same time promtail has other types that are handled as strings, so it would make it more consistent with prometheus, but less consistent with itself.

I generally prefer the method with strings rather than having a special __expr__ label or mandating the type to be an interface so that it can implement Tokenizer. Some export types like type Rules []*Config can not implement Tokenizer (without a bigger refactor) so we can only do it with strings right now. The downside of strings is lack of type safety, but tests cover some of this.

But it's a more general comment, I'm happy with the implementation in this PR.

tpaschalis

LGTM, let's merge this once @thampiotr has had a chance to take a look.

thampiotr · 2023-09-05T10:17:59Z

converter/internal/prometheusconvert/scrape.go

+				newMap := map[string]string{}
+				maps.Copy(newMap, targetMap)
+				targets = append(targets, newMap)


This was a subtle bug where the targetMap was being mutated. We need a copy here. This is why some test cases have the __address__ label changed. I believe this is the correct behaviour now.

thampiotr · 2023-09-05T10:40:18Z

Instead of adding a TODO for the static SD configs, I've implemented the conversion for them - this should work now.
Also fixed one subtle issue with map being mutated. PTAL and I'm happy for this to land @tpaschalis

#5069) * refactor promtail converter sd configs so they reuse prometheus converter functionality Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * cleanup words Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * unexport a bunch of stuff that isn't needed outside the prometheusconvert package Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * don't do SD targets in backwards order during converter Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * get rid of static SD boilerplate --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> Co-authored-by: Piotr Gwizdala <17101802+thampiotr@users.noreply.github.com> (cherry picked from commit 89b334e)

* add tracking and comparison of latest healthy content before loading a module (#5054) * add tracking and comparison of latest healthy content before loading a module * add tracking and check for arguments in module loaders Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> (cherry picked from commit 597bb6b) * add changelog version header Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * fix(http): allow override default user-agent header for http.remote (#5059) (cherry picked from commit 1eb5817) * fix(agentctl): prevent panic when validating integration-included cfg file (#5070) * fix(agentctl): prevent panic when validating integration-included cfg file Also add unit tests to prevent regression * doc(changelog): add new BUGFIXES entry (cherry picked from commit acc06f6) * loki.process: fix deadlock in case of frequent updates (#5065) Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com> (cherry picked from commit 501cf69) Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * loki.process: make deadlock test condition less sensitive (#5096) Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com> (cherry picked from commit 7b15144) * integrations/eventhandler: remove double-logging of events (#5100) This removes the info-level log line indicating that an event is logged. When agent logs are being collected, this makes the event appear as if it was double-collected, as the event appears both in the logs sent directly to Loki and in the agent logs. (cherry picked from commit 276355e) Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * Operator: update default reloader version * misc: use github.com/grafana/river (#4949) (cherry picked from commit 9a12485) Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * add more complete river identifier validation and sanitization (#4998) * use river functions to validate and sanitize river identifiers Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> (cherry picked from commit ce3fe1c) Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * Converter fix identifiers (#5102) * catch some places missing river label sanitization Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> (cherry picked from commit 6991ff2) Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * refactor promtail converter sd configs so they reuse prometheus conve… (#5069) * refactor promtail converter sd configs so they reuse prometheus converter functionality Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * cleanup words Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * unexport a bunch of stuff that isn't needed outside the prometheusconvert package Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * don't do SD targets in backwards order during converter Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * get rid of static SD boilerplate --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> Co-authored-by: Piotr Gwizdala <17101802+thampiotr@users.noreply.github.com> (cherry picked from commit 89b334e) * update test for older go version ordering Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> * Add a couple more unsupported fields to the prometheus converter Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> --------- Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com> Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com> Co-authored-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com> Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com> Co-authored-by: Robert Fratto <robertfratto@gmail.com> Co-authored-by: Craig Peterson <192540+captncraig@users.noreply.github.com>

erikbaranowski added 4 commits September 1, 2023 15:52

refactor promtail converter sd configs so they reuse prometheus conve…

3732667

…rter functionality Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

cleanup words

8625b5f

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

unexport a bunch of stuff that isn't needed outside the prometheuscon…

c979702

…vert package Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

don't do SD targets in backwards order during converter

b990eb5

Signed-off-by: erikbaranowski <39704712+erikbaranowski@users.noreply.github.com>

erikbaranowski commented Sep 1, 2023

View reviewed changes

converter/internal/promtailconvert/promtailconvert.go Outdated Show resolved Hide resolved

erikbaranowski marked this pull request as ready for review September 1, 2023 20:17

erikbaranowski requested a review from a team as a code owner September 1, 2023 20:17

wildum reviewed Sep 4, 2023

View reviewed changes

converter/internal/promtailconvert/internal/build/consul_agent.go Outdated Show resolved Hide resolved

thampiotr approved these changes Sep 5, 2023

View reviewed changes

tpaschalis approved these changes Sep 5, 2023

View reviewed changes

thampiotr reviewed Sep 5, 2023

View reviewed changes

get rid of static SD boilerplate

e283f00

thampiotr force-pushed the converter-promtail-sd-refactor branch from 186a472 to e283f00 Compare September 5, 2023 10:47

thampiotr merged commit 89b334e into main Sep 5, 2023
8 checks passed

thampiotr deleted the converter-promtail-sd-refactor branch September 5, 2023 11:58

thampiotr mentioned this pull request Sep 5, 2023

add support for discovery.triton flow component and converter #5036

Merged

4 tasks

github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor promtail converter sd configs so they reuse prometheus conve… #5069

refactor promtail converter sd configs so they reuse prometheus conve… #5069

erikbaranowski commented Sep 1, 2023 •

edited

Loading

erikbaranowski Sep 1, 2023

erikbaranowski Sep 1, 2023

erikbaranowski Sep 1, 2023

erikbaranowski Sep 1, 2023

thampiotr Sep 5, 2023

erikbaranowski Sep 1, 2023

thampiotr left a comment

thampiotr Sep 5, 2023

tpaschalis left a comment •

edited

Loading

thampiotr Sep 5, 2023

thampiotr commented Sep 5, 2023

refactor promtail converter sd configs so they reuse prometheus conve… #5069

refactor promtail converter sd configs so they reuse prometheus conve… #5069

Conversation

erikbaranowski commented Sep 1, 2023 • edited Loading

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thampiotr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpaschalis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thampiotr commented Sep 5, 2023

erikbaranowski commented Sep 1, 2023 •

edited

Loading

tpaschalis left a comment •

edited

Loading