remove rx in entity span enricher resolution by kishansairam9 · Pull Request #439 · hypertrace/hypertrace-ingester

kishansairam9 · 2023-11-08T16:52:11Z

No description provided.

codecov · 2023-11-16T08:00:30Z

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (1aea26b) 79.93% compared to head (9cf9b34) 79.87%.

Files	Patch %	Lines
.../trace/reader/attributes/DefaultValueResolver.java	54.76%	15 Missing and 4 partials ⚠️
...cher/enrichment/clients/DefaultClientRegistry.java	0.00%	7 Missing ⚠️
.../accessor/entities/DefaultTraceEntityAccessor.java	92.10%	1 Missing and 2 partials ⚠️
...ace/accessor/entities/AttributeValueConverter.java	87.50%	1 Missing ⚠️
.../accessor/entities/TraceEntityAccessorBuilder.java	0.00%	1 Missing ⚠️
...reader/attributes/DefaultTraceAttributeReader.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #439      +/-   ##
============================================
- Coverage     79.93%   79.87%   -0.06%     
+ Complexity     1421     1413       -8     
============================================
  Files           128      127       -1     
  Lines          5566     5551      -15     
  Branches        509      515       +6     
============================================
- Hits           4449     4434      -15     
+ Misses          885      879       -6     
- Partials        232      238       +6

Flag	Coverage Δ
unit	`79.87% <68.93%> (-0.06%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kishansairam9 · 2023-11-21T20:27:10Z

Blocked on fixing gradle issue on consuming attribute service which has ht bom. Looks like we need to migrate to gradle 8+ at least for fix if not complete bom, but if we're doing that might as well take care of bom

cc @mihirgt

aaron-steinfeld · 2023-11-21T21:16:05Z

Blocked on fixing gradle issue on consuming attribute service which has ht bom. Looks like we need to migrate to gradle 8+ at least for fix if not complete bom, but if we're doing that might as well take care of bom

cc @mihirgt

Even if we want to the bom now, I'd do gradle 8 first anyway. That should only take a few min.

aaron-steinfeld · 2023-11-22T16:59:39Z

-  private static final String ATTRIBUTE_SERVICE_HOST_KEY = "attribute.service.config.host";
-  private static final String ATTRIBUTE_SERVICE_PORT_KEY = "attribute.service.config.port";
+  private static final String ATTRIBUTE_SERVICE_CONFIG_KEY = "attribute.service.config";
+  private static final String ATTRIBUTE_SERVICE_HOST_KEY = ATTRIBUTE_SERVICE_CONFIG_KEY + ".host";


are host/port still used directly?

They're used to build channel

this.attributeServiceChannel = this.buildChannel( config.getString(ATTRIBUTE_SERVICE_HOST_KEY), config.getInt(ATTRIBUTE_SERVICE_PORT_KEY));

Ah I thought that went away since you removed the getter and are passing in the raw config to the client. I wonder if passing in the registry would be better so it can read the channel config in the same way? Oh well, doesn't really matter - only thing I would suggest in this case would be to remove the instance variable for channel since now it's only used locally in creating the cached client.

aaron-steinfeld · 2023-11-22T17:12:56Z

+        .filter(Optional::isPresent)
+        .findFirst()
+        .flatMap(Function.identity());


if I'm reading this correctly you've got a resolution that can return an optional and you're just trying to get the first value present, if it exists. If so, suggest:

Suggested change

.filter(Optional::isPresent)

.findFirst()

.flatMap(Function.identity());

.flatMap(Optional::stream)

.findFirst();

aaron-steinfeld · 2023-11-22T17:18:05Z

-          return new UnsupportedOperationException(String.format(message, args));
-        });
+    Stream<Optional<LiteralValue>> resolvedArguments =
+        arguments.stream().map(argument -> this.resolveProjection(valueSource, argument));


If this is the only place we're using it and we don't support it return empty, why not just throw? You're eventually throwing anyway, you're just going through an extra level of indirection. If your goal is to completely avoid the throw, collect to list (use flatmap to optional stream) and just compare size before returning.

aaron-steinfeld · 2023-11-22T17:19:57Z

+        attributeMetadataList.stream()
+            .filter(this::isEntitySourced)
+            .map(attributeMetadata -> this.resolveAttribute(attributeMetadata, trace, span))
+            .filter(Optional::isPresent)


optional stream again (there's always a better alternative for optional.get - even if it's just optional.orElseThrow - so always be wary when you find yourself pulled to it)

Thanks, wasn't aware of this neat trick with flatMap(Optional::stream)

aaron-steinfeld · 2023-11-27T13:33:21Z

-  private static final String ATTRIBUTE_SERVICE_HOST_KEY = "attribute.service.config.host";
-  private static final String ATTRIBUTE_SERVICE_PORT_KEY = "attribute.service.config.port";
+  private static final String ATTRIBUTE_SERVICE_CONFIG_KEY = "attribute.service.config";
+  private static final String ATTRIBUTE_SERVICE_HOST_KEY = ATTRIBUTE_SERVICE_CONFIG_KEY + ".host";


Ah I thought that went away since you removed the getter and are passing in the raw config to the client. I wonder if passing in the registry would be better so it can read the channel config in the same way? Oh well, doesn't really matter - only thing I would suggest in this case would be to remove the instance variable for channel since now it's only used locally in creating the cached client.

github-actions · 2023-11-27T18:00:23Z

Unit Test Results

  78 files ±0   78 suites ±0 1m 3s ⏱️ -5s
418 tests ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit f33576d. ± Comparison against base commit 1aea26b.

kishansairam9 added 6 commits November 8, 2023 22:21

remove rx in entity span enricher

0774ab6

wip

a9117c0

wip

07117e8

spotless

4953543

wrap up

928e3c9

Merge branch 'main' into remove-rx

f73ee36

This comment has been minimized.

Sign in to view

kishansairam9 commented Nov 16, 2023

View reviewed changes

Comment thread ...-reader/src/main/java/org/hypertrace/trace/accessor/entities/DefaultTraceEntityAccessor.java Outdated

kishansairam9 marked this pull request as ready for review November 16, 2023 08:04

kishansairam9 requested a review from a team as a code owner November 16, 2023 08:04

remove dep

1d2bd83

This comment has been minimized.

Sign in to view

todo finish test

31859dd

This comment has been minimized.

Sign in to view

update

202a472

This comment has been minimized.

Sign in to view

add cache metrics

cdebeb3

This comment has been minimized.

Sign in to view

nit

9645b32

This comment has been minimized.

Sign in to view

call credential fix

0015a60

This comment has been minimized.

Sign in to view

wip

079e133

kishansairam9 changed the title ~~remove rx in entity span enricher~~ remove rx in entity span enricher resolution Nov 21, 2023

remove

1ef33cb

update gradle to 8

012b274

kishansairam9 added 2 commits November 22, 2023 20:42

update

f89dd0c

Merge branch 'gradle' into remove-rx

1a81ef4

This comment has been minimized.

Sign in to view

kishansairam9 added 3 commits November 22, 2023 20:56

nit

14e6749

client utils grouper

a335c7f

Merge branch 'gradle' into remove-rx

c476735

kishansairam9 changed the base branch from main to gradle November 22, 2023 15:29

kishansairam9 requested a review from aaron-steinfeld November 22, 2023 15:30

This comment has been minimized.

Sign in to view

Base automatically changed from gradle to main November 22, 2023 15:47

update

5612453

This comment has been minimized.

Sign in to view

kishansairam9 added 2 commits November 22, 2023 22:01

nit

8213a79

nit

78cb34e

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Nov 22, 2023

View reviewed changes

update

b8486b2

This comment has been minimized.

Sign in to view

update

7566d8f

kishansairam9 requested a review from aaron-steinfeld November 23, 2023 13:16

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Nov 27, 2023

View reviewed changes

update

9cf9b34

kishansairam9 requested a review from aaron-steinfeld November 27, 2023 16:16

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Nov 27, 2023

View reviewed changes

kishansairam9 merged commit f33576d into main Nov 27, 2023

kishansairam9 deleted the remove-rx branch November 27, 2023 17:58

Conversation

kishansairam9 commented Nov 8, 2023

Uh oh!

This comment has been minimized.

This comment has been minimized.

codecov Bot commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kishansairam9 commented Nov 21, 2023

Uh oh!

aaron-steinfeld commented Nov 21, 2023

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

github-actions Bot commented Nov 27, 2023

Unit Test Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Nov 16, 2023 •

edited

Loading