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

Backend dependencies granularity for NoSQL and Messaging #2104

Merged
merged 11 commits into from
Jul 12, 2023

Conversation

stevejgordon
Copy link
Contributor

@stevejgordon stevejgordon commented Jun 6, 2023

Fixes #1754

This PR aligns more closely with the specification.

BREAKING:

  • For Redis, this removes the setting of context.db.instance and context.db.statement to align with the spec.
  • For Elasticsearch, the action has been changed to 'request' to align with the spec.
  • For Elasticsearch, db.instance is no longer set. The spec recommends that this be determined on Elastic Cloud using x-found-handling-cluster, but we don't have a way to access the headers from the client diagnostics (v7).

Notes:
For Elasticsearch, the db.statement implementation has not been changed to align with the spec as that requires more extensive changes and will be addressed as part of #1785.

Includes

  • Updates the Azure provider for Terraform to resolve issues running tests locally on Windows.
  • Adds a readme describing how to run tests locally.

@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Jun 6, 2023
@stevejgordon stevejgordon marked this pull request as draft June 6, 2023 08:26
@stevejgordon stevejgordon force-pushed the 1754-meta-659 branch 4 times, most recently from a2bb1b8 to efbdf67 Compare June 8, 2023 12:33
@stevejgordon stevejgordon marked this pull request as ready for review June 8, 2023 14:13
@stevejgordon stevejgordon changed the title [WIP] Backend dependencies granularity for NoSQL and Messaging Backend dependencies granularity for NoSQL and Messaging Jun 8, 2023
@stevejgordon
Copy link
Contributor Author

@Mpdreamz This is ready for review but is blocked as I do not currently have Azure access to run the updated Azure tests. Once these are enabled again in CI, we can run them there, or if one of us has access to run them locally, that should be sufficient for now.

BREAKING: This removes the setting of context.db.instance and
context.db.statement to align with the spec.

Adds span assertions to validate specification conditions are met for spans.
- We now attach HTTP info to the context for the main span.

BREAKING:
- The action has been changed to 'request' to align with the spec.
- The db.instance is no longer set. The spec recommends that this can
be determined on cloud using `x-found-handling-cluster` but we don't
have a way to access the headers from the client diagnostics (v7).

Notes:
`db.statement` impl has not been changed to align with the spec.
That will be future work.
Includes a rename of `DisabledOnWindowsDockerFact` to include
CI in the name so it is more explicit. These tests runs correctly on
Windows when Docker is available.
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Small note other than that LGTM

test/azure/README.md Outdated Show resolved Hide resolved
@stevejgordon stevejgordon merged commit f4dfb45 into main Jul 12, 2023
14 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Jul 12, 2023
@stevejgordon stevejgordon deleted the 1754-meta-659 branch July 12, 2023 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[META 659] Backend dependencies granularity for NoSQL and Messaging
2 participants