fix targets conversion to maps for some components#2834
Conversation
|
💻 Deploy preview available: https://deploy-preview-alloy-2834-zb444pucvq-vp.a.run.app/docs/alloy/latest/ |
thampiotr
force-pushed
the
thampiotr/fix-targets-reading-from-files
branch
from
ff47ada to
6f464ef
Compare
| select { | ||
| case <-ctx.Done(): | ||
| } | ||
| <-ctx.Done() |
There was a problem hiding this comment.
Changes to make linter happier.
| // If we were adding one, may need to clean it up too. | ||
| delete(t.toAdd, label) |
There was a problem hiding this comment.
Also changes to make linter happier.
| }) | ||
| *dst = result | ||
| return nil | ||
| case *map[string]string: |
There was a problem hiding this comment.
This was the code path required to fix the issue with
ts=2025-02-25T12:04:02.660000145Z level=error msg="failed to evaluate config" controller_path=/ controller_id="" node=prometheus.exporter.snmp.snmp err="decoding configuration: /etc/alloy/config.alloy:204:16: discovery.file.snmp.targets[0] target::ConvertInto: conversion to '*map[string]string' is not supported"
see test for more details
| } | ||
| *t = NewTargetFromLabelSet(labelSet) | ||
| return nil | ||
| case map[string]string: |
There was a problem hiding this comment.
Adding the other way around just in case we ever need it.
| t.Run("decode from target into map via scope", func(t *testing.T) { | ||
| // If not supported, this would lead to error: target::ConvertInto: conversion to '*map[string]string' is not supported | ||
| scope := vm.NewScope(map[string]interface{}{"export": NewTargetFromMap(tc.input)}) | ||
| expr, err := parser.ParseExpression("export") | ||
| require.NoError(t, err) | ||
| eval := vm.New(expr) | ||
| actualMap := map[string]string{} | ||
| require.NoError(t, eval.Evaluate(scope, &actualMap)) | ||
| require.Equal(t, tc.input, actualMap) | ||
| }) | ||
|
|
||
| t.Run("decode from map into target via scope", func(t *testing.T) { | ||
| scope := vm.NewScope(map[string]interface{}{"map": tc.input}) | ||
| expr, err := parser.ParseExpression("map") | ||
| require.NoError(t, err) | ||
| eval := vm.New(expr) | ||
| actual := Target{} | ||
| require.NoError(t, eval.Evaluate(scope, &actual)) | ||
| require.Equal(t, NewTargetFromMap(tc.input), actual) | ||
| }) |
There was a problem hiding this comment.
These were the missing tests. The issue is not obvious because we technically already have a way to convert from map to Target, because we can do map -> alloy syntax -> Target, but that is not how Alloy works, it does not figure out intermediate formats and that's probably good for performance reasons. So an explicit test of map -> Target was needed, as well as Target -> map is added for any such cases in the future.
The reason this was an issue in SNMP exporter is that in its arguments instead of Target it uses a de-facto map[string]string here: https://github.com/grafana/alloy/blob/main/internal/component/prometheus/exporter/snmp/snmp.go#L157-L160
There are other possible such cases, and they could be added in the future, so a solution of allowing this automatic conversion is preferred.
thampiotr
changed the title
* fix targets conversion to maps for some components (#2834) * update to latest walqueue (#2845) * update to latest walqueue * fix typo * fix typo * Update ckit dependency (#2847) * changelog fix --------- Co-authored-by: mattdurham <mattdurham@ppog.org> Co-authored-by: Sam DeHaan <sam.dehaan@grafana.com>
2 participants
PR Description
Fixes an issue where a component defines a
map[string]stringargument and is passed a[]discovery.Target. While conversion between map and target works in context of Alloy config literals, it requires special handling for the above case. See comments for more details.Which issue(s) this PR fixes
Fixes #2831
Notes to the Reviewer
Tested locally to repro issue raised in the ticket above:
After my fix, the isssue no longer happens.
PR Checklist