Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

moved DELETE agent_config body to query params #200281

Closed
wants to merge 4 commits into from

Conversation

bryce-b
Copy link
Contributor

@bryce-b bryce-b commented Nov 14, 2024

Summary

Resolved #198019. It's unclear why exactly this error was occurring to begin with, I didn't have luck with refactoring the ts-io parameters using body so I opted to move them to the query list.
I wasn't sure if the query parameters needed to be listed individually, or if I could re-use the serviceRt type.

Additionally, does the date on the API need to be updated?
Does this count as a breaking change?

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Unit or functional tests were updated or added to match the most common scenarios
  • 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.
  • [ x ] The PR description includes the appropriate Release Notes section, and the correct release_node:* label is applied per the guidelines

@bryce-b bryce-b marked this pull request as ready for review November 15, 2024 00:12
@bryce-b bryce-b requested a review from a team as a code owner November 15, 2024 00:12
Copy link
Contributor

@miloszmarcinkowski miloszmarcinkowski left a comment

Choose a reason for hiding this comment

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

Thanks for troubleshooting the issue and changing it to query params. I saw this is recommended approach for sending params in DELETE requests.

You need to manually update API docs and regenerate bundles. See https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/apm/docs/openapi/apm/paths/api%40apm%40settings%40agent_configuration.yaml

After you're done with bundling APM docs I'm not sure whether you need to build production docs or this is automated process. See https://github.com/elastic/kibana/blob/main/oas_docs/README.md

@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Nov 18, 2024
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

name: config.service.name,
environment: config.service.environment,
},
query: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bryce-b this is a public API, I'm not sure if we can just change its signature. I think the APM agents team are the ones using it, have you checked with them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better if I create a new API instead? If they do indeed use it, there won't be a clean way to make this change and continue supporting old agents.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest keeping the same API but implementing both query and body parameters. With query params overwriting body. That way, we stay backward compatible and can provide a working API for public (we might document only query parameters for simplicity). What do you think? @bryce-b @cauemarcondes

Copy link
Contributor

Choose a reason for hiding this comment

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

We have found the root cause of this problem. Lets close this PR, please.

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 18, 2024

💔 Build Failed

Failed CI Steps

History

@bryce-b
Copy link
Contributor Author

bryce-b commented Nov 19, 2024

Root cause of this issue described here : #200722

@bryce-b bryce-b closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:project-deploy-observability Create an Observability project release_note:fix Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UbsUx][APM] Delete "Agent Configuration" API failing on localhost
4 participants