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

Observability intake APIs #15837

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Observability intake APIs #15837

wants to merge 1 commit into from

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Feb 22, 2025

This PR makes the following changes to the OpenAPI files:

  • Adds overlays to remove patternProperties, which is not yet supported in Bump.sh
  • Adds overlays to set additionalProperties to true (if desired, we can put an explanation of the naming pattern in the descriptions and/or add more details about the supported objects there too, but this will be more effort to maintain if the original source files change).
  • Adds an overlay to remove situations where anyOf and allOf were being used to indicate when/if properties are required, since this also seems to cause problems in Bump.sh
  • Updates the make api-docs command to apply the overlays

The goal is to be able to publish this content on https://www.elastic.co/docs/api/ and not migrate the APM Server API to the new docs system.

Here's an example of a preview before and after the fixes:

Before

image

After

image

image

Next steps

I think that what's being asserted in places like this:

              allOf:
                - if:
                    properties:
                      counts:
                        type: array
                    required:
                      - counts
                  then:
                    properties:
                      values:
                        type: array
                    required:
                      - values

Is the same as what's being asserted in the description:

     properties:
        counts:
           description: >
              Counts holds the bucket counts for histogram metrics. 
              These numbers must be positive or zero.
              If Counts is specified, then Values is expected to be specified with the same number of elements, and with the same order.

So in my opinion given that this OpenAPI document is used only for documentation, it's sufficient to explain these dependencies in the text and we don't need to wait for additional functionality to publish these docs.

Copy link
Contributor

mergify bot commented Feb 22, 2025

This pull request does not have a backport label. Could you fix it @lcawl? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-9./d is the label to automatically backport to the 9./d branch. /d is the digit.
  • backport-8.x is the label to automatically backport to the 8.x branch.
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@lcawl lcawl added the backport-9.0 Automated backport to the 9.0 branch label Feb 22, 2025
@lcawl lcawl marked this pull request as ready for review February 22, 2025 01:53
@lcawl lcawl requested a review from a team as a code owner February 22, 2025 01:53
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

The files in /docs/spec/rumv3 and /docs/spec/v2 are auto-generated from code in https://github.com/elastic/apm-data/blob/main/input/elasticapm/internal/modeldecoder/generator/cmd/main.go and then copied to the apm-server repo. The spec files are not mainly used for docs but for the development workflow. They reflect the API between elastic apm agents and apm-server and are used for automated contract testing.
Changing file format to yaml, would require

  • adapting the generation code
  • adapting the automation for contract testing between any elastic apm agent and the intake protofol

@lcawl I am not convinced that switching these specs to yaml is the way forward. I'd much rather not have the API documented to this level, than changing the existing code and behaviour due to some documentation restrictions. The API docs are mainly relevant for anyone developing a custom client against it, which is not something we officially support. If needed, the spec files would still be available in the repositories, just not be part of official docs. There is also no active development planned for the APIs.

cc @raultorrecilla for awareness and further conversations.

@lcawl
Copy link
Contributor Author

lcawl commented Feb 24, 2025

I guess in that case if you still want some version of these API docs published, rather than changing to yaml and commenting out the problematic sections, we could just use overlays to remove those sections as a kind of post-processing. I will try drafting that up in this PR.

@simitt
Copy link
Contributor

simitt commented Feb 25, 2025

@lcawl maybe we could only publish the API paths and then link to the docs/spec in the apm-server repo for more details of the request body spec?

@lcawl
Copy link
Contributor Author

lcawl commented Feb 26, 2025

@lcawl maybe we could only publish the API paths and then link to the docs/spec in the apm-server repo for more details of the request body spec?

I've reset the PR to do the fixes via overlays and added details in the introduction about where to find the source files. Let me know what you think!

The API docs are mainly relevant for anyone developing a custom client against it, which is not something we officially support.

IMO that sounds like something that should be explicitly added to the info.description too. The only thing close to that in the existing docs was in https://www.elastic.co/guide/en/observability/current/apm-api-events.html "Most users do not need to interact directly with the events intake API"... but it sounds like this ought to be a stronger warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9.0 Automated backport to the 9.0 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants