Conversation
ptodev
commented
Nov 21, 2025
Contributor
Author
There was a problem hiding this comment.
The "expected" output has changed in some tests, because prometheus/alertmanager would take the global settings and apply them to each route. It would also always print some attributes like continue: false, even thought it's already set to false. I was always aware of this but since it didn't break anything I thought the benefit of using prometheus/alertmanager structs is still worth the downsides.
What changed my mind is when I saw that the file paths get modified, in an OS-inconsistent way no less. I'd rather be safe and use structs which get marshalled and unmarshalled with no side effects.
ad02d40 to
1f039a1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR vendors Alertmanager configuration types from the prometheus-operator repository instead of using the official prometheus/alertmanager package. This change resolves OS-specific behavior issues during marshal/unmarshal operations, particularly fixing a Windows test failure where absolute path detection caused inconsistent output (paths starting with / are not recognized as absolute on Windows).
Key changes:
- New vendored types in
internal/mimir/alertmanager/types.gowith custom String() and Unmarshal() methods - Updated imports across codebase from
prometheus/alertmanager/configtografana/alloy/internal/mimir/alertmanager - Modified String() method signature to return
(string, error)instead ofstringfor better error handling
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/mimir/alertmanager/types.go | New vendored Alertmanager config types from prometheus-operator with custom marshaling methods and exported fields |
| internal/mimir/client/client.go | Updated import to use vendored alertmanager types |
| internal/mimir/client/alerts.go | Modified to handle new String() method signature that returns error |
| internal/mimir/client/alerts_test.go | Updated to use new Unmarshal function and YAMLEq for comparison |
| internal/component/mimir/alerts/kubernetes/events.go | Updated imports and error handling for new String() signature |
| internal/component/mimir/alerts/kubernetes/events_test.go | Updated test to use new types and YAMLEq comparison |
| internal/component/mimir/alerts/kubernetes/alerts.go | Updated to use new Unmarshal function and dereference returned pointer |
| internal/mimir/client/testdata/alertmanager/response.good.yml | Removed default values that are no longer emitted by new marshaling |
| internal/mimir/client/testdata/alertmanager/conf.good.yml | Removed comments and added default http_config values |
| internal/cmd/integration-tests-k8s/tests/mimir-alerts-kubernetes/testdata/expected_1.yml | Updated expected output to match new marshaling behavior |
| internal/cmd/integration-tests-k8s/tests/mimir-alerts-kubernetes/testdata/expected_2.yml | Updated expected output to match new marshaling behavior |
| Makefile | Minor documentation formatting improvement |
jharvey10
approved these changes
Nov 21, 2025
Contributor
jharvey10
left a comment
There was a problem hiding this comment.
I'm definitely not an alerts expert, but the changes look pretty straightforward to me.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jharvey10
pushed a commit
that referenced
this pull request
Nov 21, 2025
* Vendor Alertmanager config * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Merged
jharvey10
added a commit
that referenced
this pull request
Nov 24, 2025
* ci: sync publish workflow with main * Vendor Alertmanager config (#4905) * Vendor Alertmanager config * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * beyla: add meta_cache_address to beyla.ebpf.attributes.kubernetes (#4871) * beyla: add meta_cache_address to beyla.ebpf.attributes.kubernetes * chore: update changelog * chore: lint --------- Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com> * Upgrade Beyla component to 2.7.7 (#4891) * upgrade beyla to 2.7.7 * update to Beyla 2.7.8 * update docs * fix(pyroscope.ebpf): Use meter noop in otel-ebpf-profiler (#4920) With a recent change, the default meter provider was no longer initialized. This change disables the otel-ebpf-profiler internal metrics to avoid warnings like this to appear: ``` WARN[0121] Invalid metric id 102, skipping WARN[0121] Invalid metric id 272, skipping ``` * bump rc version in changelog --------- Co-authored-by: Paulin Todev <paulin.todev@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Stephen Lang <skl@users.noreply.github.com> Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com> Co-authored-by: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Co-authored-by: Christian Simon <simon@swine.de>
dehaansa
pushed a commit
to madhub/alloy
that referenced
this pull request
Dec 10, 2025
* Vendor Alertmanager config * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#3448 would take the Alertmanager (AM) config from the AM repo:
This PR instead copies AM config from the prometheus-operator repo. The benefit in doing this is that there are less side effects during marshal/unmarshal. The main driver is that the official struct from
prometheus/alertmanagerdoesn't even behave consistently across oeprating systems. For example, a test currently fails on Windows:This is because the code checks if
templatesis an absolute path and modifies it if it's not. On windows, paths starting with/do not look like absolute paths. This is a problem for Alloy since Alloy could be running on a different OS from Alertmanager.