jaeger icon indicating copy to clipboard operation
jaeger copied to clipboard

Visualize uninstrumented services in the dependency diagrams

Open nidhey27 opened this issue 1 year ago • 3 comments

Which problem is this PR solving?

  • Closes #3804
  • Replaces #4853

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

  • [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
  • [x] I have signed all commits
  • [x] I have added unit tests for the new functionality
  • [x] I have run lint and test steps successfully
    • for jaeger: make lint test
    • for jaeger-ui: yarn lint and yarn test

nidhey27 avatar Dec 30 '23 04:12 nidhey27

Ref PR: https://github.com/jaegertracing/jaeger/pull/4853

nidhey27 avatar Dec 30 '23 04:12 nidhey27

+1, we are awaiting this feature. Currently the deep dependency graph is not showing the relationship with the databases and other non-instrumented services, for the given service.

kannan-ak avatar Feb 19 '24 11:02 kannan-ak

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.76%. Comparing base (a72dfc3) to head (d031261).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5062      +/-   ##
==========================================
- Coverage   95.20%   93.76%   -1.44%     
==========================================
  Files         343      343              
  Lines       16813    16846      +33     
==========================================
- Hits        16006    15795     -211     
- Misses        608      869     +261     
+ Partials      199      182      -17     
Flag Coverage Δ
badger 10.43% <0.00%> (-0.04%) :arrow_down:
cassandra-3.x 18.31% <0.00%> (-0.07%) :arrow_down:
cassandra-4.x 18.31% <0.00%> (-0.07%) :arrow_down:
elasticsearch-5.x 20.81% <0.00%> (-0.10%) :arrow_down:
elasticsearch-6.x 20.81% <0.00%> (-0.13%) :arrow_down:
elasticsearch-7.x 20.87% <0.00%> (-0.12%) :arrow_down:
elasticsearch-8.x 21.10% <0.00%> (-0.03%) :arrow_down:
grpc ?
kafka 10.13% <0.00%> (-0.04%) :arrow_down:
opensearch-1.x 20.90% <0.00%> (-0.07%) :arrow_down:
opensearch-2.x 20.93% <0.00%> (-0.05%) :arrow_down:
unittests 91.74% <100.00%> (+0.01%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 22 '24 03:04 codecov[bot]

This is a dupe of #4853 which, as you can see, was a very long conversation. This PR is a 10 line change. It's very frustrating for the maintainers who need to spend a ton of time fixing such a small issue. I spoke to the author as well and explained this decision we are making. The decision is that this will not be merged as it would need to be rebased and once again create a lot of work for the team. Sorry.

jkowall avatar Jul 30 '24 13:07 jkowall