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

Visualize uninstrumented services in the dependency diagrams #4853

Closed
wants to merge 8 commits into from
Closed

Visualize uninstrumented services in the dependency diagrams #4853

wants to merge 8 commits into from

Conversation

nidhey27
Copy link

@nidhey27 nidhey27 commented Oct 17, 2023 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • Updated GetDependencies to identify and include client spans without corresponding server spans, ensuring a complete dependency graph.
  • Implemented updateServiceDependencyLinks to streamline the update of service dependencies, reducing code duplication.
  • Enabled the representation of uninstrumented services in the dependency graph by inferring names from client spans lacking server spans.

How was this change tested?

  • Introduced TestInferredServiceDependency to specifically evaluate the functionality of inferred service dependencies in the scenario of uninstrumented services.
  • Created a client span without a corresponding server span and asserted the correct formation of dependencies, ensuring the inclusion of inferred services in the dependency graph.
  • Employed assertions to confirm the presence and accuracy of inferred service dependencies, ensuring they are adequately represented with the correct attributes and relationships.

Checklist

@nidhey27 nidhey27 requested a review from a team as a code owner October 17, 2023 06:33
@nidhey27 nidhey27 requested a review from jkowall October 17, 2023 06:33
yurishkuro
yurishkuro reviewed Oct 17, 2023
View reviewed changes
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
yurishkuro
yurishkuro reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

👍

yurishkuro
yurishkuro reviewed Oct 26, 2023
View reviewed changes
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory.go Outdated Show resolved Hide resolved
plugin/storage/memory/memory_test.go Outdated Show resolved Hide resolved
@nidhey27 nidhey27 changed the title [WIP] Visualize uninstrumented services in the dependency diagrams Visualize uninstrumented services in the dependency diagrams Nov 20, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (84e7b69) 95.55% compared to head (0c7b1e9) 96.43%.

❗ Current head 0c7b1e9 differs from pull request most recent head ef30383. Consider uploading reports for the commit ef30383 to get more accurate results

Files Patch % Lines
plugin/storage/memory/memory.go 94.23% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4853      +/-   ##
==========================================
+ Coverage   95.55%   96.43%   +0.88%     
==========================================
  Files         313      304       -9     
  Lines       18160    18089      -71     
==========================================
+ Hits        17352    17444      +92     
+ Misses        647      510     -137     
+ Partials      161      135      -26
Flag Coverage Δ
cassandra-3.x ?
cassandra-4.x ?
elasticsearch-5.x ?
elasticsearch-6.x ?
elasticsearch-7.x ?
elasticsearch-8.x ?
grpc-badger ?
kafka ?
opensearch-1.x ?
opensearch-2.x ?
unittests 96.43% <94.23%> (+3.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Nov 20, 2023
@yurishkuro
Copy link
Member

I suggest you take a look at hotrod's graph with this logic, it looks odd. Originally HotROD instrumentation was intentionally tweaked to produce a nice graph since we didn't have this derived client nodes displaying. Perhaps you can undo some of those changes and/or change instrumentation to follow semantic conventions better (e.g. so that Redis is recognized). There's also an odd loop around mysql, it shouldn't be there.

image

yurishkuro
yurishkuro approved these changes Dec 28, 2023
View reviewed changes
examples/hotrod/services/customer/database.go
@@ -75,9 +74,8 @@ func newDatabase(tracer trace.Tracer, logger log.Factory) *database {
func (d *database) Get(ctx context.Context, customerID int) (*Customer, error) {
d.logger.For(ctx).Info("Loading customer", zap.Int("customer_id", customerID))

ctx, span := d.tracer.Start(ctx, "SQL SELECT", trace.WithSpanKind(trace.SpanKindClient))
_, span := d.tracer.Start(ctx, "SQL SELECT")
Copy link
Member

Choose a reason for hiding this comment

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

don't follow the reasoning. Both ctx and span kind were appropriate

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is I don't agree with the change, it doesn't make sense to me. This particular code was already correct.

Copy link
Author

@nidhey27 nidhey27 Dec 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The reason behind removing ctx & spanKind was to discard extra span named "inferred::SQL SELECT"👇🏻
image

nidhey27 added 8 commits December 29, 2023 12:44
add: artificial server span and test for Uninstrumented Services
3ad3763 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
update: refined service dependency links without creating artificial …
f5e293a 
…spans

Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
add: function for dynamic service name inference
d0b2e98 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
remove: db.statement & http.url tag values
1b9f5e2 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
update: leaf span handling and tests for service name inference
3e912c2 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
improved dependency link handling for various span types
9309d72 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
address review comment
d60bab3 
Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
refactor: streamline database.Get method, redis.FindDriverIDs & redis…
ef30383 
….GetDriver

Signed-off-by: nidhey27 <nidhey.indurkar@infracloud.io>
@nidhey27 nidhey27 mentioned this pull request Dec 30, 2023
Closed
4 tasks
@yurishkuro
Copy link
Member

Replaced by #5062

@yurishkuro yurishkuro closed this Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@jkowall jkowall Awaiting requested review from jkowall jkowall is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Visualize uninstrumented services in the dependency diagrams
2 participants
@nidhey27 @yurishkuro