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

Validation for metadata.service not working #4141

Closed
simitt opened this issue Sep 3, 2020 · 3 comments · Fixed by #4142
Closed

Validation for metadata.service not working #4141

simitt opened this issue Sep 3, 2020 · 3 comments · Fixed by #4142
Assignees
Labels

Comments

@simitt
Copy link
Contributor

simitt commented Sep 3, 2020

Turns out that the current way of writing the validation rules for metadata.service.* fields in the json-schema is not working properly.
Instead of writing e.g. "properties.agent.required": ["name", "version"] one would need to expand to objects like

            "properties": {
                "agent": {
                    "type": "object",
                    "required": [
                        "name",
                        "version"
                    ]
                }
             }

or probably even better, just copy & paste the whole service definition and adapt the required fields.

Unfortunately this has also been missed in the package_tests as tests for metadata are very sparse.

Following fields should be changed to the key being required and value not being allowed to be null:

  • metadata.service.name (currently key required but value allowed to be null) ,
  • metadata.service.agent (currently key required but value allowed to be null), * metadata.service.agent.name,metadata.service.agent.version,metadata.service.runtime.name,metadata.serice.runtime.version, metadata.service.language.name (currently key not required and value allowed to be null)

Technically this is a bug and we should fix it; but it would potentially break agents that might not send the required fields.

I suggest to create a fix and ask agents to run their tests against the fixed json schema to see if any current agent versions would break when fixing this.

The bug exists since 7.0.

@simitt simitt added the bug label Sep 3, 2020
@simitt
Copy link
Contributor Author

simitt commented Sep 3, 2020

One more note - since we use the same schema validation for the metadata sent for profiles we would also need to check that this endpoint doesn't break.

simitt added a commit to simitt/apm-server that referenced this issue Sep 3, 2020
Validations on metadata.service.* fields were not run correctly, changing the schema to properly run validations and
adding package tests for verification.

fixes elastic#4141
@simitt
Copy link
Contributor Author

simitt commented Sep 3, 2020

As pointed out by @jalvz the original request from the agents (@felixbarny ) was to make metadata.service.* information optional, but only if provided per event. I don't think we can build that on a json schema level though, as multiple events need to be merged with each other, so this check could only happen in the decoder logic.

@axw
Copy link
Member

axw commented Sep 4, 2020

One more note - since we use the same schema validation for the metadata sent for profiles we would also need to check that this endpoint doesn't break.

Profiling is still experimental, so no big deal if we have to update the Go agent to match the change.

simitt added a commit that referenced this issue Sep 14, 2020
Validations on metadata.service.* fields were not run correctly, changing the schema to properly run validations and
adding package tests for verification.

fixes #4141
simitt added a commit to simitt/apm-server that referenced this issue Sep 14, 2020
Validations on metadata.service.* fields were not run correctly, changing the schema to properly run validations and
adding package tests for verification.

fixes elastic#4141
simitt added a commit that referenced this issue Sep 14, 2020
)

Validations on metadata.service.* fields were not run correctly, changing the schema to properly run validations and
adding package tests for verification.

fixes #4141
@axw axw removed the [zube]: Done label Sep 16, 2020
@simitt simitt mentioned this issue Oct 14, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants