Added s3-prefix to S3 destination.service.resource

[JonasKunz]

This PR is essentially a followup of #683 regarding the second point in this comment:

• Should context.destination.service.resource be changed to include an 's3/' prefix?
  Currently the S3 spec has service.target = { type: 's3', name: '$bucketNameIfAvailable' }. This means that following the usual inference of context.destination.service.resource from context.service.target we would expect "s3/$bucketName" (or "s3" if there is no bucket name, e.g. for the "ListBuckets" API call) rather than the currently specified "$bucketNameIfAvailable".

The reason why this issue came back on the table is that @AlexanderWert encountered it in form of a defect in the UI (see elastic/apm-agent-java#2849).

Even though context.destination.service.resource is deprecated, it is likely to stay around for a while and impact UI features. Therefore we need to still fix it imo.

To my knowledge, adding the s3/ would have the following negative consequences:

• It would break the history of the S3-Spans / metrics for users relying on context.destination.service.resource
• If users happen to run agents both with and without this fix (for same or different languages), the same S3-buckets can appear twice in the service map (with and without s3-prefix)

This list might not be exhaustive, feel free to comment if you come up with additional concerns.

In my opinion these negative consequences are acceptable, because I would classify this change as a bugfix.
We should however explicitly highlight this change in the agent changelogs when implementing this spec update.

• May the instrumentation collect sensitive information, such as secrets or PII (ex. in headers)?
  • Yes
    • Add a section to the spec how agents should apply sanitization (such as sanitize_field_names)
  • No
    • Why? -> Diff says it all
  • n/a
• Create PR as draft
• Approval by at least one other agent
• Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
  • Remove PM from reviewers if impact on product is negligible
  • Remove agents from reviewers if the change is not relevant for them
• Approved by at least 2 agents + PM (if relevant)
• Merge after 7 days passed without objections
  To auto-merge the PR, add /schedule YYYY-MM-DD to the PR description.
• Create implementation issues through the meta issue template (this will automate issue creation for individual agents)
• If this spec adds a new dynamic config option, add it to central config.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-01-02T05:54:00.595+0000

• Duration: 3 min 10 sec

[z1c0]

LGTM.
Should we add a test case for that in https://github.com/elastic/apm/blob/main/tests/agents/json-specs/service_resource_inference.json though?

[JonasKunz]

LGTM. Should we add a test case for that in https://github.com/elastic/apm/blob/main/tests/agents/json-specs/service_resource_inference.json though?

I gave it a shot and added the following test locally:

  {
    "span": {
      "exit": "true",
      "type": "storage",
      "subtype": "s3",
      "context": {
        "service": {
          "target": {
            "type": "s3",
            "name": "bucket-name"
          }
        }
      }
    },
    "expected_resource": "s3/bucket-name",
    "expected_service_target": {
      "type": "s3",
      "name": "bucket-name"
    },
    "failure_message": "For s3, the service.resource should follow the common pattern"
  }

At least for the Java agent this test doesn't add any value, because it passes successfully without having implemented the new s3/ prefix behaviour. I think the reason is that the special case is not implemented in the inference logic, but inside the s3 instrumentation. Inside the instrumentation we explicitly assign the service.resource field, therefore the inference doesn't get executed.

@z1c0 if you still see value in adding this test (e.g. for .Net) I can add it. Also please feel free to make any modifications to my suggested test.

[z1c0]

You are absolutely right @JonasKunz - the test case does not add any value (same for .NET).
Thanks for trying this out though.