feat: enhance the support for dropping filter by kotharironak · Pull Request #288 · hypertrace/hypertrace-ingester

kotharironak · 2021-11-18T15:35:22Z

Description

This PR, enhance the support for dropping the span at entry-level. There are certain issues in the existing one.

It only supports equal operation
it's not easily readable
uses a certain string delimiter making its error-prone

e.g of existing drop filter criteria
["k1:v1, k2:v2", "k3:v3]

As we can see, it is supported as an array of strings that are considered as OR expressions. The string supports AND operation via :.

New Approach:

It will be readable JSON structure
will supports EQ, CONTAINS, EXISTS and NEQ operator

e.g

[
	[
		{
			"tagKey": "http.path",
			"operator": "EQ",
			"tagValue": "abcdef"
		},
		{
			"tagKey": "http.url",
			"operator": "CONTAINS",
			"tagValue": "val"
		},
		{
			"tagKey": "grpc.tag",
			"operator": "EXISTS"
		}
	],
	[
		{
		"tagKey": "tenant-id",
		"operator": "EQ",
		"tagValue": "xyz"
		}
	]
]

As it can be seen, its array of OR (AND [Filters]) expression.

Testing

Added unit tests and extended topology test driver test too

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

kotharironak · 2021-11-18T15:36:56Z

@avinashkolluru @satish-mittal @ravisingal This PR extends the new approach. Once, this will be available, the existing one will be removed as follow-up PR.

codecov · 2021-11-18T15:37:37Z

Codecov Report

Merging #288 (8b73714) into main (431d690) will increase coverage by 0.24%.
The diff coverage is 89.09%.

@@             Coverage Diff              @@
##               main     #288      +/-   ##
============================================
+ Coverage     79.20%   79.44%   +0.24%     
- Complexity     1239     1265      +26     
============================================
  Files           111      112       +1     
  Lines          4880     4934      +54     
  Branches        442      452      +10     
============================================
+ Hits           3865     3920      +55     
+ Misses          813      812       -1     
  Partials        202      202

Flag	Coverage Δ
unit	`79.44% <89.09%> (+0.24%)`	⬆️

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

Impacted Files	Coverage Δ
...ace/core/spannormalizer/jaeger/SpanDropFilter.java	`88.88% <88.88%> (ø)`
...ertrace/core/spannormalizer/jaeger/SpanFilter.java	`88.54% <88.88%> (+0.20%)`	⬆️
.../spannormalizer/jaeger/JaegerSpanPreProcessor.java	`84.61% <100.00%> (+11.53%)`	⬆️
...izer/jaeger/JaegerSpanToLogRecordsTransformer.java	`91.30% <0.00%> (+2.17%)`	⬆️

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 431d690...8b73714. Read the comment docs.

avinashkolluru · 2021-11-23T05:33:16Z

+    switch (filter.getOperator()) {
+      case EQ:
+        return tags.containsKey(filter.getTagKey())
+            && StringUtils.equals(tags.get(filter.getTagKey()).getVStr(), filter.getTagValue());


@kotharironak We may have to look into the process tags too. See below

public Optional<String> getTenantId( Map<String, KeyValue> spanTags, Map<String, KeyValue> processTags) { return Optional.ofNullable(processTags.get(tenantIdKey)) .or(() -> Optional.ofNullable(spanTags.get(tenantIdKey))) .map(KeyValue::getVStr); }

cc: @laxman-traceable @ravisingal

I think the process tags are meant for resources, do we want to drop span based on resource criteria? So, far we are dropping based on span attribute filters, and with that, we are able to filter existing scenarios, right? Are there some scenario?

@ravisingal @avinashkolluru I have added the support for processTags too. So, now the filter will check in the union of two tags (processTags, spanTags).

Total Tags = Process Tags + Span Tags will be used for matching a filter.

…ags too

avinashkolluru · 2021-11-23T12:35:18Z


+    if (anySpanDropFiltersMatch(spanDropFilters, tags, processTags)) {
+      if (DROPPED_SPANS_RATE_LIMITER.tryAcquire()) {
+        LOG.info("Dropping span: [{}] with drop filters: [{}]", span, spanDropFilters);


This will log too much. I think we should move this to debug. Even above it should move to debug.
I'm assuming once we migrate the configs to this new format, we'll get rid of the above.

avinashkolluru · 2021-11-23T12:44:44Z

+                        .collect(Collectors.toList());
+                  })
+              .collect(Collectors.toList());
+      LOG.info("Json String for Span drop criterion: {}", this.spanDropFilters);


toString() missing in SpanDropFilter class

Its not a Json right?

No, it is List<List>, move the logging in the top before parsing in case of any failure we know what has failed.

findingrish · 2021-11-23T15:20:33Z

    }

+    if (anySpanDropFiltersMatch(spanDropFilters, tags, processTags)) {
+      if (DROPPED_SPANS_RATE_LIMITER.tryAcquire()) {


nit: can you put this block inside a debugger enabled check?

There is to string not any eval, should it still matter?

Wanted to avoid the extra ratelimiter check

Ah! I see b'cause of tryAcquire, can do this.

github-actions · 2021-11-23T16:26:22Z

Unit Test Results

  74 files ±0   74 suites ±0 1m 4s ⏱️ -3s
392 tests +3 392 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit efa5d17. ± Comparison against base commit 431d690.

feat: enhance the support for dropping filter

89c99cb

kotharironak requested review from a team, avinashkolluru, findingrish, ravisingal and satish-mittal November 18, 2021 15:35

Merge branch 'main' into enhancing-drop-criteria

94223ce