Adds resource enrichment by Harnoor-se7en · Pull Request #58 · razorpay/hypertrace-ingester

Harnoor-se7en · 2022-09-22T12:32:34Z

Adds ResourceAttributeEnricher to attach infra metadata to span tags.

codecov · 2022-09-22T12:34:57Z

Codecov Report

Merging #58 (9482e87) into rzp_main (92601e7) will increase coverage by 0.04%.
The diff coverage is 83.33%.

@@              Coverage Diff               @@
##             rzp_main      #58      +/-   ##
==============================================
+ Coverage       79.45%   79.50%   +0.04%     
- Complexity       1311     1329      +18     
==============================================
  Files             118      119       +1     
  Lines            5271     5322      +51     
  Branches          481      486       +5     
==============================================
+ Hits             4188     4231      +43     
- Misses            865      870       +5     
- Partials          218      221       +3

Flag	Coverage Δ
unit	`79.50% <83.33%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
...rg/hypertrace/traceenricher/util/EnricherUtil.java	`70.21% <78.57%> (+6.87%)`	⬆️
...nrichment/enrichers/ResourceAttributeEnricher.java	`85.29% <85.29%> (ø)`

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

suddendust · 2022-09-23T04:11:14Z

+  @Override
+  public void enrichEvent(StructuredTrace trace, Event event) {
+    try {
+      if (resourceAttributesToAdd == null || resourceAttributesToAdd.isEmpty()) {


Why will this ever be null?

suddendust · 2022-09-23T04:40:53Z

+      if (resourceAttributesToAdd == null || resourceAttributesToAdd.isEmpty()) {
+        return;
+      }
+      if (event.getResourceIndex() < 0) {


Add all of these checks to a method validatePreconditions?

suddendust · 2022-09-23T04:41:15Z

+        return;
+      }
+      for (String resourceAttributeKey : resourceAttributesToAdd) {
+        Optional<AttributeValue> resourceAttribute =


Rename to resourceAttributeMaybe

suddendust · 2022-09-23T04:41:45Z

+            attributeValue -> attributeMap.putIfAbsent(resourceAttributeKey, attributeValue));
+      }
+    } catch (Exception e) {
+      LOGGER.error("Exception while enriching event with resource attributes.", e);


Log some span metadata?

suddendust · 2022-09-23T04:42:56Z


+  public static Optional<org.hypertrace.core.datamodel.AttributeValue> getAttribute(
+      Attributes attributes, String key) {
+    return Optional.ofNullable(attributes)


Use filter with findFirst

Didn't get you, findFirst in the map?

See my latest comment on this. You can stream the map entries, add a filter and then do a findFirst. This returns an Optional by default. Cleaner IMO but this is fine too.

suddendust · 2022-09-23T04:43:40Z

+
+    when(event.getAttributes()).thenReturn(Attributes.newBuilder().build());
+    when(event.getAttributes().getAttributeMap()).thenReturn(null);
+    resourceAttributeEnricher.enrichEvent(null, event);


What are you asserting in this test?

error logs or any exception.

Where's the assertion? A test method should only throw an AssertionError, as a result of an assertion failing. You can do:

try { resourceAttributeEnricher.enrichEvent(null, event); } catch (Exception e) { throw new AssertionError("...") }

yeah, so I think this test is useless as I am not throwing any exceptions. removing it then.

suddendust · 2022-09-23T04:46:47Z

+        attributeMapSize = event.getAttributes().getAttributeMap().size();
+      }
+      resourceAttributeEnricher.enrichEvent(trace, event);
+      if (event.getAttributes() != null && event.getAttributes().getAttributeMap() != null) {


Do you really need these checks if you're constructing the trace yourself?

Yes. These checks were not before and some of the events of custom trace did not have these attributes which makes sense to test on different edge cases.

suddendust

First iteration, please address the changes.

suddendust

LGTM

github-actions · 2022-09-28T08:47:57Z

Unit Test Results

  77 files +1   77 suites +1 57s ⏱️ -11s
401 tests +3 399 ✔️ +3 2 💤 ±0 0 ❌ ±0

Results for commit 161035d. ± Comparison against base commit 92601e7.

Harnoor-se7en added 2 commits September 22, 2022 12:32

Adds resource attribute enricher

aba9d8d

Adds test for resource attribute enricher

5a3a1f1

Harnoor-se7en changed the title ~~Add infra metadata~~ Add process tags to spans Sep 22, 2022