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

Fix metadata.service.* fields requirements in json schema #4142

Merged
merged 7 commits into from
Sep 14, 2020

Conversation

simitt
Copy link
Contributor

@simitt simitt commented Sep 3, 2020

Motivation/summary

Validations on metadata.service.* fields were not run correctly. This PR changes the schema to properly run validations for v2 and rum-v3:

  • metadata.service.name,metadata.service.agent.name: key required, value must be non-empty string ,
  • metadata.service.agent.version: key must be sent, value must not be null
  • metadata.service.runtime.name,metadata.service.runtime.version: keys must be sent, values must not be null if metadata.service.runtime is sent
  • metadata.service.language.name: keys must be sent, values must not be null if metadata.service.language is sent

The PR also fixes an issue where metadata.service.cloud.* fields that are not required were not allowed to be sent as null values.

Checklist

- [ ] I have signed the Contributor License Agreement.

I have considered changes for:
- [ ] documentation
- [ ] logging (add log lines, choose appropriate log selector, etc.)
- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)

How to test these changes

Sending payloads where metadata.service.agent.name,metadata.service.agent.version,metadata.service.runtime.name,metadata.serice.runtime.version,metadata.service.language.name are missing or set to null and ensure payload is invalid.

Related issues

fixes #4141

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 simitt requested a review from a team September 3, 2020 12:02
@apmmachine
Copy link
Contributor

apmmachine commented Sep 3, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #4142 updated]

  • Start Time: 2020-09-14T07:51:15.906+0000

  • Duration: 42 min 20 sec

Test stats 🧪

Test Results
Failed 0
Passed 3543
Skipped 145
Total 3688

Steps errors

Expand to view the steps failures

  • Name: Compress

    • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

    • Duration: 0 min 0 sec

    • Start Time: 2020-09-14T08:06:11.247+0000

    • log

  • Name: Compress

    • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

    • Duration: 0 min 0 sec

    • Start Time: 2020-09-14T08:19:16.521+0000

    • log

  • Name: Test Sync

    • Description: ./script/jenkins/sync.sh

    • Duration: 3 min 53 sec

    • Start Time: 2020-09-14T08:00:34.840+0000

    • log

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, but we should probably wait until we've got a resolution on #4141 (comment)

@simitt
Copy link
Contributor Author

simitt commented Sep 8, 2020

Update: I added a minLen=1 requirement for the metadata.service.name and metadata.service.agent.name - since sending an empty string should also be considered a bug.

@simitt
Copy link
Contributor Author

simitt commented Sep 8, 2020

@elastic/apm-agent-devs as just discussed please check the requirements for the current agent versions and let me know if this bugfix causes issues, otherwise I'll merge by the end of the week.

@simitt simitt requested a review from axw September 8, 2020 15:03
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm wondering why we make version required but don't also require it to be non-empty. Seems like all fields should be either optional or required and non-empty.

@simitt
Copy link
Contributor Author

simitt commented Sep 9, 2020

I didn't want to introduce too many changes with this as it will be deployed as bugfix; the reason I went for adding minLen to service.name and service.agent.name is that they are more crucial to the APM solution.

@eyalkoren
Copy link
Contributor

Java agent should be OK with this change.

  • metadata.service.name - there should be fallback always, including when setting an empty string.
  • metadata.service.agent.name: hardcoded "java"
  • metadata.service.agent.version: either the valid version or "unknown"
  • metadata.service.runtime.name: hardcoded "Java"
  • metadata.service.runtime.version: the value of the java.version system property, should never be null
  • metadata.service.language.name: hardcoded "Java"

As far as I can see, it is the case since GA.
Since the only configurable option is service.name, I think it shouldn't break anything.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Looks good from RUM agent side.

@simitt simitt merged commit 79ec0ed into elastic:master Sep 14, 2020
simitt added a commit to simitt/apm-server that referenced this pull request 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 pull request 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 simitt deleted the fix-metadata-service-requirements branch October 6, 2020 10:12
@jalvz jalvz self-assigned this Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation for metadata.service not working
7 participants