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

Use the resource auto-inference to set context.destination.service.resource for JDBC spans (#1913) #1928

Closed
wants to merge 43 commits into from

Conversation

tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented Jul 22, 2021

What does this PR do?

Fixes #1913

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation

@apmmachine
Copy link
Contributor

apmmachine commented Jul 22, 2021

❕ Build Aborted

The PR is not allowed to run in the CI yet

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

  • Reason: The PR is not allowed to run in the CI yet

  • Start Time: 2022-01-28T10:45:56.903+0000

  • Duration: 7 min 34 sec

  • Commit: e7b6ed0

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@eyalkoren
Copy link
Contributor

Yes, I understood what you requested, but this is what I referred to as a patch.
A real config will probably not be such, but more something like: use_instance_for_db_resource, and it is relevant to all agents, not only Java. We wouldn't want to let make our users think about auto-inference and how it works, but on what this config would change.

So, let us consider that. Maybe we decide to add this as an internal config.

@tobiasstadler
Copy link
Contributor Author

Ok, so should I change my implementation and just add the instance to context.destination.service.resource in JdbcHelper if the internal config option use_instance_for_db_resource is true?

@SylvainJuge SylvainJuge added the community Issues and PRs created by the community label Jul 27, 2021
…db_resource and add the instance name during span creation.
@tobiasstadler
Copy link
Contributor Author

@eyalkoren I implemented your suggestion

@eyalkoren
Copy link
Contributor

@tobiasstadler sorry for the long pauses between responses, we are extremely busy these days and I was OOO lately. Let me try pushing elastic/apm#473 forward first, and if we can't get that, we will consider a Java-specific configuration. Normally we try to avoid that, as it make future decisions more complicated.

@tobiasstadler
Copy link
Contributor Author

Ok, thanks you @eyalkoren

@felixbarny
Copy link
Member

@SylvainJuge is working on a concept that plans to use service.target.name and service.target.type from ECS instead.

@felixbarny felixbarny closed this Jan 28, 2022
@tobiasstadler tobiasstadler deleted the fix-1913 branch January 28, 2022 12:03
@tobiasstadler
Copy link
Contributor Author

@SylvainJuge / @felixbarny Is there any (rough) timeline (you can share) when your concept will be available?

@SylvainJuge
Copy link
Member

Hi @tobiasstadler, we aim to start working on this in the next couple of months. As usual any estimate should be taken with a pinch shovel of salt.

Given it involves changing things in multiple projects: all agents, apm-server and apm-ui it's really hard to give you any estimate when it will be available in practice. However, it's something that is an important on a product level, so it is really something that is expected to happen in the short/mid-term time-frame.

@tobiasstadler
Copy link
Contributor Author

Ok, thank You for the information. I guess I have to wait then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java community Issues and PRs created by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resource auto-inference is not working for JDBC spans
5 participants