Skip to content

Conversation

@kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Dec 30, 2020

While working on the issue - hypertrace/hypertrace#128, I have observed the following topology sort for enrichers.

enricher-sorting-order

As we can see from the code - https://github.com/hypertrace/hypertrace-trace-enricher/blob/435d21caab170c5b8d207785204b61839089a599/hypertrace-trace-enricher-impl/src/main/java/org/hypertrace/traceenricher/enrichment/enrichers/BackendEntityEnricher.java#L112 - that backend entity enricher depended on attribute ApiAttribute.API_ATTRIBUTE_ID in EndpointEnricher. So, adding the dependency.

@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #93 (37bc490) into main (70f66f3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main      #93   +/-   ##
=========================================
  Coverage     68.94%   68.94%           
  Complexity      820      820           
=========================================
  Files            84       84           
  Lines          3475     3475           
  Branches        367      367           
=========================================
  Hits           2396     2396           
  Misses          935      935           
  Partials        144      144           
Flag Coverage Δ Complexity Δ
unit 68.94% <ø> (ø) 0.00 <ø> (ø)

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


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 70f66f3...37bc490. Read the comment docs.

@jcchavezs
Copy link
Contributor

@kotharironak I wonder how this did not fail and more so, how can we prevent this kind of things in the future.

@tim-mwangi
Copy link
Contributor

tim-mwangi commented Dec 30, 2020

@kotharironak I wonder how this did not fail and more so, how can we prevent this kind of things in the future.

unit tests :) That actually verify expectations

@jcchavezs
Copy link
Contributor

unit tests :) That actually verify expectations

But isn't this a dependency issue? Shouldn't this appear on compilation? I am just wondering.

In any case if this is because of lack of unit tests I would quickly add one before merge it.

port = ${?ENTITY_SERVICE_PORT_CONFIG}
}
dependencies = ["DefaultServiceEntityEnricher"]
dependencies = ["DefaultServiceEntityEnricher", "EndpointEnricher"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why do we have all this dependency information as part of config. Do we need to frequently change this order based on environment etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we change this based on the environment but we want to be able to enable/disable a given enricher.

@kotharironak
Copy link
Contributor Author

unit tests :) That actually verify expectations

But isn't this a dependency issue? Shouldn't this appear on the compilation? I am just wondering.

In any case if this is because of lack of unit tests I would quickly add one before merge it.

Currently, it is config driven dependency so, I think, it's hard to check at compilation time. But, yes, some sort of test case help here. I will create a follow-up PR for this.

@kotharironak kotharironak merged commit e652fc0 into main Jan 4, 2021
@kotharironak kotharironak deleted the backend-as-service branch January 4, 2021 09:16
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.

7 participants