Skip to content

Conversation

@lloydmeta
Copy link
Member

@lloydmeta lloydmeta commented Jul 10, 2025

Summary

This PR adds a discriminator to the SLO timeslice metric types in the OpenAPI specification. The discriminator is introduced to address a critical issue encountered with client generation and data marshalling; specifically, when handling polymorphic metric types like percentile, which is a superset of the basic field metric type. Without a discriminator, clients (such as the Terraform provider) will incorrectly unmarshal data, defaulting fields like percentile to zero due to ambiguous type resolution.

Why is this needed?

  • OpenAPI code generation: When generating clients from the OpenAPI schema, the lack of a discriminator can cause unmarshalling logic to use first-type-match wins. For example, metrics intended to be of the percentile type are instead parsed as the basic field metric type, losing the percentile field (see discussion). This leads to incorrect default values and change detection issues in downstream systems.
  • Correct type mapping: The discriminator allows reliable mapping
  • The Elastic Stack TF provider change has been merged with this fix anyway - not having this in the upstream harms Kibana clients and in addition, will break the provider if they pull in new versions of your spec without this fix

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
  • Review the backport guidelines and apply applicable backport:* labels.

Identify risks

n/na

Related

@lloydmeta lloydmeta requested a review from a team as a code owner July 10, 2025 10:15
@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Jul 10, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@cla-checker-service
Copy link

cla-checker-service bot commented Jul 10, 2025

💚 CLA has been signed

@lloydmeta lloydmeta added backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Jul 10, 2025
CLA
Signed-off-by: lloydmeta <[email protected]>
@lloydmeta lloydmeta enabled auto-merge (squash) July 10, 2025 10:26
@simianhacker
Copy link
Member

Thanks for this contribution! To help with the review process and make this change easier for other contributors to understand, could you add a bit more context to the PR description? It would be helpful to know "What problem this this PR solves or the issues you are running into?"

This additional context will make it much easier for reviewers to understand the motivation and impact of the change. Thanks!

@lloydmeta
Copy link
Member Author

This additional context will make it much easier for reviewers to understand the motivation and impact of the change. Thanks!

Done.

Also note that there is some urgency here in the last point I added to the description

  • The Elastic Stack TF provider change has been merged with this fix anyway - not having this in the upstream harms Kibana clients and in addition, will break the provider if they pull in new versions of your spec without this fix

Copy link
Contributor

@florent-leborgne florent-leborgne left a comment

Choose a reason for hiding this comment

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

Docs change LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

@lloydmeta
Copy link
Member Author

@elastic/obs-ux-management-team @elastic/core-docs your review is needed to get this merged (kind reminder there is some urgency) 🙏🏼

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@lloydmeta lloydmeta merged commit 7d0592c into elastic:main Jul 16, 2025
13 checks passed
@lloydmeta lloydmeta deleted the openapi/add-discrimiator-to-timeslice-slo-types branch July 16, 2025 04:53
Bluefinger pushed a commit to Bluefinger/kibana that referenced this pull request Jul 22, 2025
…227400)

## Summary

Adds a discriminator to the SLO timeslice metric type in OpenAPI.

Signed-off-by: lloydmeta <[email protected]>
Signed-off-by: lloydmeta <[email protected]>
kertal pushed a commit to kertal/kibana that referenced this pull request Jul 25, 2025
…227400)

## Summary

Adds a discriminator to the SLO timeslice metric type in OpenAPI.

Signed-off-by: lloydmeta <[email protected]>
Signed-off-by: lloydmeta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants