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

Add spec for instrumenting Azure services #426

Merged
merged 15 commits into from
Jul 19, 2021

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Mar 24, 2021

Relates: elastic/apm-agent-dotnet#1225

This PR introduces a spec to start capturing details specific to instrumenting Azure services.

This commit introduces a spec to start capturing details
specific to instrumenting Azure services.
@russcam russcam requested review from a team as code owners March 24, 2021 06:36
@apmmachine
Copy link

apmmachine commented Mar 24, 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 preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-19T04:22:46.415+0000

  • Duration: 3 min 4 sec

  • Commit: 6a68966

Trends 🧪

Image of Build Times

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Document how the context.destination.service.resource is defined. There's some high-level definition in the messaging spec but I think it makes sense to be more concrete here and use the Azure service bus-specific names. For example, is it the topic or the queue name?

specs/agents/tracing-instrumentation-azure.md Outdated Show resolved Hide resolved
@AlexanderWert AlexanderWert linked an issue Mar 25, 2021 that may be closed by this pull request
Copy link
Contributor

@mikker mikker left a comment

Choose a reason for hiding this comment

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

This is WONDERUL, @russcam! 👏

This commit adds additional blob rules that
have been derived from the .NET SDK.
| --------- | --------- | ------ | ----- | ------- |
| `context.destination.address` | yes | URL scheme and host | | `https://namespace.servicebus.windows.net/` |
| `context.destination.service.name` | yes | azureservicebus | | |
| `context.destination.service.resource` | yes | azureservicebus/`<Queue>`\|`<Topic and Subscription>` | | `azurequeue/myqueue`, `azureservicebus/mytopic/Subscriptions/mysubscription` |
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the same rule as for regular queues? "azureservicebus/$context.message.queue.name"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Messages can be sent either to a queue or to a topic.

Is context.destination.service.resource just used in the service map UI? Depending on the level of granularity desired, perhaps the Service bus namespace might be a good contender for context.destination.service.resource? If so, would we still want to capture queue or topic separately?

Copy link
Member

Choose a reason for hiding this comment

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

It's mainly used for the service map and the dependencies table. Also, the APM Server rolls up metrics with the resource as a dimension. See also #435.

specs/agents/tracing-instrumentation-azure.md Outdated Show resolved Hide resolved
specs/agents/tracing-instrumentation-azure.md Outdated Show resolved Hide resolved
specs/agents/tracing-instrumentation-azure.md Outdated Show resolved Hide resolved

| APM field | Required? | Format | Notes | Example |
| --------- | --------- | ------ | ----- | ------- |
| `span.name` | yes | `AzureBlob <OperationName> <ResourceName>` | Pascal case Operation name | `AzureBlob Upload foo/bar/baz` |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixbarny thoughts on excluding the <ResourceName> from the span name? If it were to be removed, is there another appropriate field to capture it?

Copy link
Member

Choose a reason for hiding this comment

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

Random thought: The Azure Blob container is perhaps close to the equivalent of the bucket name that is spec'd to be used in the span.name for S3 instrumentation. However, IIUC there could, at least theoretically, be a cardinality issue with that because Azure documents that a "storage account can include an unlimited number of containers", where as, AWS limits to 100 buckets to start (which can be raised to 1000).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

container is pretty much Azure's bucket 😄 If AWS spec uses bucket in the span name, it seems reasonable to use container in the span name, where applicable


| APM field | Required? | Format | Notes | Example |
| --------- | --------- | ------ | ----- | ------- |
| `span.name` | yes | `AzureTable <OperationName> <ResourceName>` | Pascal case Operation name | `AzureTable Insert tablename` |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixbarny thoughts on excluding the <ResourceName> from the span name? If it were to be removed, is there another appropriate field to capture it?


| APM field | Required? | Format | Notes | Example |
| --------- | --------- | ------ | ----- | ------- |
| `span.name` | yes | `AzureFile <OperationName> <ResourceName>` | Pascal case Operation name | `AzureFile Create directoryname` |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixbarny thoughts on excluding the <ResourceName> from the span name? If it were to be removed, is there another appropriate field to capture it?

russcam added a commit to elastic/apm-agent-dotnet that referenced this pull request May 26, 2021
@mikker
Copy link
Contributor

mikker commented Jun 3, 2021

russcam added 2 commits June 4, 2021 09:07
This commit updates context.destination.address to
use just the URL host
russcam added a commit to russcam/apm-agent-dotnet that referenced this pull request Jun 4, 2021
russcam added a commit to elastic/apm-agent-dotnet that referenced this pull request Jun 4, 2021
russcam added a commit to elastic/apm-agent-dotnet that referenced this pull request Jun 4, 2021
@russcam
Copy link
Contributor Author

russcam commented Jul 15, 2021

Last call to review/comment on this PR. If there are none received in the next 24 hours, I'll be merging in.

@russcam russcam merged commit b338fe9 into elastic:master Jul 19, 2021
@russcam russcam deleted the feature/tracing-instrumentation-azure branch July 19, 2021 04:30
trentm pushed a commit to trentm/apm that referenced this pull request Oct 6, 2021
This commit introduces a spec to start capturing details
specific to instrumenting Azure services.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 410] SPEC - Azure services instrumentation
8 participants