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

Fix lag metric for grouper #283

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

samarth-gupta-traceable
Copy link
Contributor

@samarth-gupta-traceable samarth-gupta-traceable commented Nov 8, 2021

Description

I see 2 potential issues with current arrival lag reporting:

  1. original objective on 'arrival.lag' was to report lag at start of job. overtime this has been changed it seems, and currently it reports lag when trace is about to be emitted.
    hence we are currently reporting lag including the time span spent in rawspangrouper.

  2. another issue is that, we are determining the lag based on trace start time which is set of grouper as currenttime. which does not give the correct value of lag. as lag represents how much delay as span has faced till now in the data pipeline.

this PR it trying to address only the 2nd point. as of now.

For now i am of the opinion we can consider the metrics been reported as just lag instead of arrival lag. Grouper is already reporting at end of job. I have made changes to move enricher reporting also to end of enrichment.
any thoughts @ravisingal @surajpuvvada ?

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #283 (5c8a554) into main (30f0a7b) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

❗ Current head 5c8a554 differs from pull request most recent head 56ea3d7. Consider uploading reports for the commit 56ea3d7 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main     #283      +/-   ##
============================================
- Coverage     79.07%   79.06%   -0.02%     
  Complexity     1231     1231              
============================================
  Files           110      110              
  Lines          4861     4863       +2     
  Branches        440      441       +1     
============================================
+ Hits           3844     3845       +1     
  Misses          813      813              
- Partials        204      205       +1     
Flag Coverage Δ
unit 79.06% <75.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...race/core/rawspansgrouper/TraceEmitPunctuator.java 74.13% <75.00%> (-0.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30f0a7b...56ea3d7. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@samarth-gupta-traceable
Copy link
Contributor Author

@rish691 @kotharironak I am not exactly sure what does getReceivedTimeMillis on a span represent. does it tell us when was span received at span normalizer or platform agent or tracing agent ?

@github-actions
Copy link

github-actions bot commented Nov 8, 2021

Unit Test Results

  73 files  ±0    73 suites  ±0   1m 4s ⏱️ +3s
390 tests ±0  390 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 56ea3d7. ± Comparison against base commit 30f0a7b.

@samarth-gupta-traceable
Copy link
Contributor Author

samarth-gupta-traceable commented Nov 8, 2021

@rish691 @kotharironak I am not exactly sure what does getReceivedTimeMillis on a span represent. does it tell us when was span received at span normalizer or platform agent or tracing agent ?

looks like it is time when span was received at normalizer. https://github.com/hypertrace/hypertrace-ingester/blob/main/span-normalizer/span-normalizer/src/main/java/org/hypertrace/core/spannormalizer/jaeger/JaegerSpanNormalizer.java#L116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant