Skip to content

Conversation

@kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Feb 23, 2022

Description

There are cases when data received by the platform can be late. This can cause problems in an underlying store like pinot, etc.
In either case, we needed a configuration to drop late coming data.

As part of this PR, we are adding the below configuration at the span-normalizer component.

processor:
  lateArrivalThresholdDuration: 5m

The above config tells that if the span's start_time_millis and span received time at span-normalizer is higher than 5m, it will be dropped.

Testing

  • Added topology and unit tests
  • Added helm config and tested locally by generating template

Screenshot 2022-02-23 at 8 56 46 PM

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #306 (d857a48) into main (42cd6e7) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #306      +/-   ##
============================================
+ Coverage     79.70%   79.77%   +0.07%     
- Complexity     1297     1302       +5     
============================================
  Files           118      118              
  Lines          5159     5177      +18     
  Branches        467      469       +2     
============================================
+ Hits           4112     4130      +18     
  Misses          837      837              
  Partials        210      210              
Flag Coverage Δ
unit 79.77% <100.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
.../spannormalizer/jaeger/JaegerSpanPreProcessor.java 88.57% <100.00%> (+3.95%) ⬆️

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 42cd6e7...d857a48. Read the comment docs.

@github-actions

This comment has been minimized.


private Duration configureLateArrivalThreshold(Config jobConfig) {
Duration configuredThreshold = jobConfig.getDuration(LATE_ARRIVAL_THRESHOLD_CONFIG_KEY);
Duration minThreshold = Duration.of(30, ChronoUnit.SECONDS);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was suggested by @laxman-traceable . I think, he has suggestions for max value too. But, I have kept it open.

Duration spanArrivalDelay =
Duration.of(Math.abs(spanProcessedTime - spanStartTime), ChronoUnit.MILLIS);

if (spanStartTime > 0 && spanArrivalDelay.compareTo(lateArrivalThresholdDuration) > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This condition spanStartTime > 0 is required as default proto value for the field is 0. So, if there are spans when it is not set value, they are considered valid for backward compatibility.

String tenantId = "tenant-" + random.nextLong();
Map<String, Object> configs = new HashMap<>(getCommonConfig());
configs.putAll(Map.of("processor", Map.of()));
configs.putAll(Map.of("processor", Map.of("late.arrival.threshold.duration", "1d")));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the config - late.arrival.threshold.duration - is part of the mandatory config, need to update all existing tests.

private Duration configureLateArrivalThreshold(Config jobConfig) {
Duration configuredThreshold = jobConfig.getDuration(LATE_ARRIVAL_THRESHOLD_CONFIG_KEY);
Duration minThreshold = Duration.of(30, ChronoUnit.SECONDS);
if (minThreshold.compareTo(configuredThreshold) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a mandatory config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @laxman-traceable suggested that.


private Duration configureLateArrivalThreshold(Config jobConfig) {
Duration configuredThreshold = jobConfig.getDuration(LATE_ARRIVAL_THRESHOLD_CONFIG_KEY);
Duration minThreshold = Duration.of(30, ChronoUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this can be defined as private static final minLateArrivalThreshold = Duration.ofSeconds(30);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit 90ccd36 into main Feb 24, 2022
@kotharironak kotharironak deleted the drop-late-arrival-span branch February 24, 2022 04:52
@github-actions
Copy link

Unit Test Results

  76 files  ±0    76 suites  ±0   1m 19s ⏱️ +3s
400 tests +3  400 ✔️ +3  0 💤 ±0  0 ❌ ±0 

Results for commit 90ccd36. ± Comparison against base commit 42cd6e7.

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.

4 participants