Skip to content

Conversation

wasfree
Copy link

@wasfree wasfree commented Sep 4, 2025

… be set

Fix for #1085 and #1035

If aggregation in metric_custom_indicator is set to doc_count it's not allowed to specifie "field"

Copy link

cla-checker-service bot commented Sep 4, 2025

💚 CLA has been signed

@wasfree wasfree closed this Sep 4, 2025
@wasfree wasfree reopened this Sep 4, 2025
@wasfree wasfree force-pushed the bugfix/fix-metric-custom-indicator-doc-count branch 2 times, most recently from 2d91f69 to 9bc6700 Compare September 4, 2025 13:54
… be set

Signed-off-by: Nick Metz <nick-metz@web.de>
@wasfree wasfree force-pushed the bugfix/fix-metric-custom-indicator-doc-count branch from 9bc6700 to de1943d Compare September 4, 2025 14:14
@tobio
Copy link
Member

tobio commented Sep 5, 2025

There's a few more changes required here, one of which is potentially large:

  • Update where we populate the API model to optionally read the field (here and here). We'd likely also want to add some validation in so the provider errors if field is set when for doc_type aggregations, or unset for other aggregation types.
  • Update the generated SLO client, the spec used to generate this client lists field as required, and so it's always being serialised in the API request body leading to 400's. That means updating slo-spec.yml with the current spec, regenerating the client with make generate-slo-client and fixing any errors caused by the potential model changes. Done in Update generated SLO client.  #1303
  • Adding an acceptance test to slo_test.go verifying the actual bug fix.

@tobio
Copy link
Member

tobio commented Sep 15, 2025

@wasfree I've updated the SLO client as part of some other work, which should simplify the rest of this task if you've still got some time to look at it.

@kpatticha
Copy link

@tobio I’m also running into this limitation, are there any alternative workarounds until it gets merged?

@tobio
Copy link
Member

tobio commented Oct 1, 2025

Nope there's no workaround available unfortunately.

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