feat: add service target fields support to azure module #1280
Conversation
Sets service target fields in the azure module. See https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-azure.md
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
module/apmazure/storage.go
Outdated
| @@ -108,6 +108,10 @@ func (p *apmPipeline) Do( | |||
| span.Context.SetDestinationService(apm.DestinationServiceSpanContext{ | |||
| Resource: rpc.subtype() + "/" + rpc.storageAccountName(), | |||
| }) | |||
| span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{ | |||
| Type: rpc.subtype(), | |||
| Name: rpc.storageAccountName(), | |||
There was a problem hiding this comment.
Maybe we need to add another method to the azureRPC interface for this? The spec only mentions setting these fields for Azure Service Bus and Azure Queue Storage, and for those the name should be queue / topic.
Can you please have a dig into the spec, and/or check with the other agent devs, to find out what if anything we should set for other Azure services?
There was a problem hiding this comment.
That's correct, right now there's no explicit guidance on what those fields should be:
See elastic/apm#661
See elastic/apm#646
From what I could gather, Type should fallback to subtype if empty.
There was a problem hiding this comment.
OK, so in the mean time let's just make sure to use the queue name for Queue storage. Currently we're using the account name, which does not match the updated spec.
We might want to use this to parse the queue name out of request URLs: https://github.com/Azure/azure-storage-queue-go/blob/636801874cdd196abace508ef19c3edca7ed0564/azqueue/parsing_urls.go#L24
There was a problem hiding this comment.
Understood! 👍
I went with a lighter solution but looking at this a bit more, it seems the current name and resource fields are using the account name (https://github.com/elastic/apm-agent-go/blob/main/module/apmazure/storage.go#L109 and https://github.com/elastic/apm-agent-go/blob/main/module/apmazure/queue.go#L36) but they should be using the queue name too.
I've updated the service target fields and left those for a followup PR.
Omit the field if there is no clear specification. Use the queuename for azurequeue
module/apmazure/storage.go
Outdated
| @@ -108,6 +108,10 @@ func (p *apmPipeline) Do( | |||
| span.Context.SetDestinationService(apm.DestinationServiceSpanContext{ | |||
| Resource: rpc.subtype() + "/" + rpc.storageAccountName(), | |||
| }) | |||
| span.Context.SetServiceTarget(apm.ServiceTargetSpanContext{ | |||
| Type: rpc.subtype(), | |||
| Name: rpc.storageAccountName(), | |||
There was a problem hiding this comment.
OK, so in the mean time let's just make sure to use the queue name for Queue storage. Currently we're using the account name, which does not match the updated spec.
We might want to use this to parse the queue name out of request URLs: https://github.com/Azure/azure-storage-queue-go/blob/636801874cdd196abace508ef19c3edca7ed0564/azqueue/parsing_urls.go#L24
The queue name is retrieved from the url: /myqueue/messages See https://docs.microsoft.com/en-us/rest/api/storageservices/queue-service-rest-api
target name is now using the queue name instead of the account name. Tests have been updated to use a correct url as specified in the rest api documentation. See https://docs.microsoft.com/en-us/rest/api/storageservices/queue-service-rest-api
strings Cut is not available on older versions of Go.
…hmark-reporting * upstream/main: (25 commits) docs: update correct env flag for loglevel (elastic#1299) fix: readd deprecated span_frames_min_duration option as fallback for older configuration (elastic#1297) feat: rename span_frames_min_duration to span_stack_trace_min_duration (elastic#1285) test: verify Ubuntu cgroup line parsing for container ID (elastic#1293) tracer: Parse global labels per tracer (elastic#1290) feat: update sns span.name to reflect the current spec (elastic#1286) fix: expand k8s pod discovery regex (elastic#1288) test: add testcase for sqs delete_batch operation (elastic#1283) docs: document ELASTIC_APM_SERVER_CA_CERT_FILE (elastic#1289) fix: reformat code with go 1.19 to fix ci failure (elastic#1284) feat: add service target fields support to elasticsearch module (elastic#1281) fix: use the correct destination resource and name for azure queue (elastic#1282) feat: add service target fields support to azure module (elastic#1280) feat: add service target fields support to aws module (elastic#1278) feat: add service target fields support to sql module (elastic#1279) synchronize json schema specs (elastic#1260) fix: make sure at least one of the service target fields is sent (elastic#1277) docs: add link to release-notes-2.x (elastic#1271) feat: add service target fields (elastic#1274) perf: skip tracestate regex validation for es vendor key (elastic#1275) ...
Sets service target fields in the azure module.
See https://github.com/elastic/apm/blob/main/specs/agents/tracing-instrumentation-azure.md