Skip to content
This repository was archived by the owner on Nov 14, 2023. It is now read-only.

Service Internal Latency #12

Merged
merged 19 commits into from
Sep 8, 2022
Merged

Service Internal Latency #12

merged 19 commits into from
Sep 8, 2022

Conversation

suddendust
Copy link

@suddendust suddendust commented Jun 2, 2022

Spec: https://docs.google.com/document/d/1wCSS3BRRrbwmnJBDvPNaL3O8ENQgTkD8iLlUTdnlMYI/edit

Edit: Have taken a better approach:

  1. We create the API trace graph from the structured trace. This gives us the graph of all API nodes in the graph.
  2. We iterate through each of the API nodes.
  3. For each API node, we get the entry event and all outbound calls to API.
  4. We calculate the sum of all outbound calls and subtract it from the entry API node's duration.
  5. We add the attribute.

@suddendust suddendust added enhancement New feature or request donotmerge labels Jun 2, 2022

@Override
public void enrichEvent(StructuredTrace trace, Event event) {
var apiBoundaryType = event.getEnrichedAttributes().getAttributeMap()
Copy link

@Harnoor-se7en Harnoor-se7en Jun 2, 2022

Choose a reason for hiding this comment

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

maybe we can add these checks
check
check
or we can wrap code inside try catch so that this enricher can be skipped.

@Override
public void enrichEvent(StructuredTrace trace, Event event) {
var apiBoundaryType = event.getEnrichedAttributes().getAttributeMap()
.get(API_BOUNDARY_TYPE_ATTR_NAME).getValue();
Copy link

@Harnoor-se7en Harnoor-se7en Jun 2, 2022

Choose a reason for hiding this comment

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

I think according to ApiBoundaryTypeAttributeEnricher, API_BOUNDARY_TYPE_ATTR_NAME key will not always be set.

private static boolean isEligibleForSubtraction(Event childEvent) {
var apiBoundaryType = childEvent.getEnrichedAttributes().getAttributeMap()
.get(API_BOUNDARY_TYPE_ATTR_NAME);
return apiBoundaryType != null && apiBoundaryType.getValue().equals(EXIT_BOUNDARY_TYPE);

Choose a reason for hiding this comment

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

Is this the complete condition check?

.filter(InternalServiceLatencyEnricher::isEligibleForSubtraction)
.map(InternalServiceLatencyEnricher::getEventDuration)
.reduce(0L, Long::sum);
event.getAttributes().getAttributeMap()

Choose a reason for hiding this comment

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

why not enrichedAttributes?
if this is used then maybe we can create a new method with checks like in this in parent.

Copy link

@Harnoor-se7en Harnoor-se7en left a comment

Choose a reason for hiding this comment

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

Looks good but I think there are some edge cases where the flow can break. Though wouldn't cause any issue except causing a high error rate/logs.

EnrichedSpanConstants.getValue(BoundaryTypeValue.BOUNDARY_TYPE_VALUE_ENTRY);

@Override
public void enrichEvent(StructuredTrace trace, Event event) {

Choose a reason for hiding this comment

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

add some unit test as well pls

Copy link
Author

Choose a reason for hiding this comment

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

Added

Copy link

@Harnoor-se7en Harnoor-se7en left a comment

Choose a reason for hiding this comment

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

LGTM

List<ApiNode<Event>> apiNodeList = apiTraceGraph.getApiNodeList();
for (ApiNode<Event> apiNode : apiNodeList) {
List<ApiNodeEventEdge> edges = apiTraceGraph.getOutboundEdgesForApiNode(apiNode);
int edgeDurationSum = 0;

Choose a reason for hiding this comment

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

nit: long

Copy link
Author

Choose a reason for hiding this comment

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

Why? Will this sum ever exceed 2,147,483,647ms? That's around 25 days.

var entryApiBoundaryEvent = entryApiBoundaryEventMaybe.get();
var entryApiBoundaryEventDuration = getEventDuration(entryApiBoundaryEvent);
try {
entryApiBoundaryEvent.getAttributes().getAttributeMap()

Choose a reason for hiding this comment

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

should it be enrichedAttributes or this is fine?

Copy link
Author

Choose a reason for hiding this comment

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

Should be here as we want to see these attributes on the UI. Enriched attributes are not shown.

@suddendust suddendust added the DevTestingStarted DevTestingStarted label Jun 12, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2022

Codecov Report

Merging #12 (dc0bc4a) into rzp_main (1e6a206) will decrease coverage by 0.14%.
The diff coverage is 62.50%.

@@              Coverage Diff               @@
##             rzp_main      #12      +/-   ##
==============================================
- Coverage       79.60%   79.46%   -0.15%     
- Complexity       1306     1311       +5     
==============================================
  Files             117      118       +1     
  Lines            5223     5273      +50     
  Branches          474      482       +8     
==============================================
+ Hits             4158     4190      +32     
- Misses            853      865      +12     
- Partials          212      218       +6     
Flag Coverage Δ
unit 79.46% <62.50%> (-0.15%) ⬇️

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

Impacted Files Coverage Δ
.../enrichedspan/constants/EnrichedSpanConstants.java 62.50% <ø> (+12.50%) ⬆️
...richers/ServiceInternalProcessingTimeEnricher.java 62.50% <62.50%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@suddendust suddendust merged commit c910426 into rzp_main Sep 8, 2022
@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Unit Test Results

  76 files  +1    76 suites  +1   1m 9s ⏱️ +7s
398 tests +2  396 ✔️ ±0  2 💤 +2  0 ❌ ±0 

Results for commit c910426. ± Comparison against base commit 1e6a206.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DevTestingStarted DevTestingStarted donotmerge enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants