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.

Sign up for GitHub

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

Jump to bottom

Add new fields to service to track new data type and name #1279

Merged
merged 14 commits into from
Jul 28, 2022
Merged

Add new fields to service to track new data type and name #1279

merged 14 commits into from
Jul 28, 2022

Conversation

jaggederest
Copy link
Contributor

@jaggederest jaggederest commented Jun 16, 2022 •
edited
Loading

What does this pull request do?

Adds new fields to agent-server protocol, specifically span.context.service.target.type and span.context.service.target.name

Why is it important?

These fields will be used in the future as the correct way to capture service details in the agent-server API

Checklist

  • I have signed the Contributor License Agreement.
  • My code follows the style guidelines of this project (See .rubocop.yml)
  • I have rebased my changes on top of the latest main branch
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related issues

Relates #1244 (partially completes)

@jaggederest jaggederest linked an issue Jun 16, 2022 that may be closed by this pull request
[META 621] Improve Granularity for SQL Databases #1244
Open
4 tasks
@apmmachine
Copy link
Contributor

apmmachine commented Jun 16, 2022 •
edited
Loading

💚 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: 2022-07-28T08:12:19.779+0000

  • Duration: 25 min 25 sec

Test stats 🧪

Test Results
Failed 0
Passed 44648
Skipped 80
Total 44728

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

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

@apmmachine
Copy link
Contributor

apmmachine commented Jun 16, 2022 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (1/1) 💚
Files 99.219% (127/128) 👍 0.006
Classes 99.219% (127/128) 👍 0.006
Lines 68.514% (2955/4313) 👎 -0.076
Conditionals 100.0% (0/0) 💚

@jaggederest jaggederest marked this pull request as ready for review June 20, 2022 03:37
@jaggederest jaggederest requested a review from estolfo June 20, 2022 03:37
estolfo
estolfo requested changes Jun 20, 2022
View reviewed changes
lib/elastic_apm/span/context/service.rb Outdated Show resolved Hide resolved
estolfo
estolfo requested changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@estolfo estolfo left a comment

Choose a reason for hiding this comment

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

We also need to add in the functionality to actually capture these values for SQL databases.

@jaggederest jaggederest removed a link to an issue Jun 23, 2022
[META 621] Improve Granularity for SQL Databases #1244
Open
4 tasks
@jaggederest jaggederest force-pushed the 1244-improve-granularity-for-sql-databases branch from f238a31 to 47168ea Compare June 23, 2022 17:28
estolfo
estolfo requested changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@estolfo estolfo left a comment

Choose a reason for hiding this comment

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

I think it would make sense to add tests for the database instrumentations, right?
Also, don't we have to keep the existing fields for compatibility with the UI?

@estolfo
Copy link
Contributor

estolfo commented Jul 4, 2022

After reviewing the spec again for making these changes, there are a few adjustments to this PR:

  • Only the sequel spy needs to be updated, as it's only for SQL databases. A spec is in progress for NoSQL databases and messaging libraries.
  • We need to get the target name for the sequel instrumentation. I don't know without doing more research how to extract it, but it looks like from here, sequel has a number of adapters for various SQL databases. You'll probably have to access the adapter and possibly parse the connection string to get the target name field.

@jaggederest jaggederest force-pushed the 1244-improve-granularity-for-sql-databases branch from 4944b7f to 9aaaafb Compare July 19, 2022 13:36
@jaggederest
Copy link
Contributor Author

Also, don't we have to keep the existing fields for compatibility with the UI?

I was uncertain about that. It seems like the spec says "Infer from those new fields the value of span.destination.service.resource and keep sending it." - does that mean that other span.destination.service.* parameters should be removed and only resource should be retained? It's fairly easy to do either way, I just don't completely understand the wording there.

@estolfo
Copy link
Contributor

estolfo commented Jul 22, 2022

I think at this point, we are only supposed to add the new fields and not stop sending any existing fields.

@jaggederest
Copy link
Contributor Author

I think at this point, we are only supposed to add the new fields and not stop sending any existing fields.

Just for posterity's sake, the existing sequel spy did not send destination.service.name or destination.service.type before this PR, so they are not being sent. Had to double check that to make sure I didn't accidentally delete them.

@jaggederest jaggederest requested a review from estolfo July 26, 2022 06:48
estolfo
estolfo requested changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@estolfo estolfo left a comment

Choose a reason for hiding this comment

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

I noticed a typo so proposed a correction but I also suggest adding a test that would have caught the typo.

lib/elastic_apm/span.rb Outdated Show resolved Hide resolved
@jaggederest jaggederest requested a review from estolfo July 27, 2022 05:48
@estolfo
Copy link
Contributor

estolfo commented Jul 27, 2022

I just saw that the tests are failing but I approve the code changes once they are passing. Let's get the tests passing before merging. I'm happy to do another code review once the test failures are fixed. Thanks!

@jaggederest jaggederest merged commit 72f82ea into main Jul 28, 2022
@jaggederest jaggederest deleted the 1244-improve-granularity-for-sql-databases branch July 28, 2022 10:50
estolfo pushed a commit that referenced this pull request Mar 2, 2023
@jaggederest @estolfo
Add new fields to service to track new data type and name (#1279)
94aa819 
* Add new fields to service to track new data type and name

* Add missing reference to service in context

* Testing for new service.target.name and ~.type fields

* Update spies to use new service.target.name/type

* Remove changes to non-sequel spies and attempt to get database name from connection

* Add new fields to service to track new data type and name

* Add missing reference to service in context

* Update spies to use new service.target.name/type

* Update specs for final sql granularity changes

* Reset azure_storage_table spy - no changes needed in this pr

* Add one more test for span context serializer

* less restrictive test for sequel service.target.name

* Add a spec covering set_destination

* Update db_name logic in sequel spy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estolfo estolfo estolfo approved these changes

Assignees
No one assigned
Labels
agent-ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaggederest @apmmachine @estolfo