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

Shared JSON spec for span types/subtypes #1812

Merged
merged 31 commits into from
Jun 2, 2021

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented May 12, 2021

Description

Creates and enforces JSON spec for all the span types/subtypes that are used in the Java agent

This is a preliminary step to having a shared spec applied to all APM agents.

Related PR for all agents : elastic/apm#443

Checklist

  • gather the list of all values currently reported by tests for the Java agent to make test pass.
  • decide how to handle values for test only
    • sharing those across agent does not make sense
    • applying a wildcard or prefix-based approach hardcoded before the check could help (for example do not validate every foo.* type.
    • Solution: Allow manual opt-out on a per-test-method basis
  • decide how to handle null (not provided) or "" (empty) sub-types:
    • should we make them optional on some known types ? making them optional on every type ?
    • Solution (1): normalize to null instead of empty and treat sub-type as optional
    • Solution (2): normalize to null instead of empty and allow null on some types (not all of them).
  • decide how to handle "open ended" span subtypes:
    • process type that uses binary name as sub-type (or find a proper way to fix it).
    • Solution: remove subtype that was using binary name, it's already provided in name
    • template type that uses template engine name as sub-type (and is also very platform-specific)
    • Solution: enforce only the type and not subtype as it's really platform-specific
  • decide what to do with OpenTracing & OpenTelemetry bridges
    • remap cache.redis to db.redis for consistency
    • use custom as default span type instead of unknown

@apmmachine
Copy link
Contributor

apmmachine commented May 12, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #1812 updated

  • Start Time: 2021-06-02T13:21:59.807+0000

  • Duration: 51 min 12 sec

  • Commit: 7c27ff3

Test stats 🧪

Test Results
Failed 0
Passed 2189
Skipped 18
Total 2207

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2189
Skipped 18
Total 2207

@SylvainJuge
Copy link
Member Author

See elastic/apm#443 PR for adding this to shared specifications.

@SylvainJuge SylvainJuge requested a review from eyalkoren May 31, 2021 07:29
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

LGTM, few minor comments and one question about the spec itself - for a given type, is null subtype allowed when the spec defines a list of subtypes?

* Sets all optional checks to their default value (enabled), should be used as a shortcut to reset mock reporter state
* after/before using it for a single test execution
*/
public void resetChecks() {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@SylvainJuge SylvainJuge requested a review from eyalkoren June 2, 2021 09:43
@SylvainJuge SylvainJuge marked this pull request as ready for review June 2, 2021 13:05
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

LGTM! One minor suggestion

@SylvainJuge SylvainJuge merged commit a837d43 into elastic:master Jun 2, 2021
@SylvainJuge SylvainJuge deleted the span-type-subtype-spec branch June 2, 2021 14:59
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.

3 participants