Support service correlation for mirroring spans by sanket-mundra · Pull Request #431 · hypertrace/hypertrace-ingester

sanket-mundra · 2023-10-12T19:31:29Z

Description

BREAKING CHANGE: Added a new state store in raw-spans-grouper: peer-identity-to-span-metadata-store

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

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

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

codecov · 2023-10-12T19:34:46Z

Codecov Report

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

Comparison is base (70faf7e) 79.88% compared to head (0912315) 79.53%.
Report is 32 commits behind head on main.

❗ Current head 0912315 differs from pull request most recent head 3a217cf. Consider uploading reports for the commit 3a217cf to get more accurate results

Files	Patch %	Lines
...cher/enrichment/clients/DefaultClientRegistry.java	0.00%	37 Missing ⚠️
.../trace/reader/attributes/DefaultValueResolver.java	54.76%	15 Missing and 4 partials ⚠️
...rtrace/core/rawspansgrouper/RawSpansProcessor.java	87.70%	4 Missing and 11 partials ⚠️
...race/core/rawspansgrouper/TraceEmitPunctuator.java	76.47%	9 Missing and 3 partials ⚠️
...rawspansgrouper/validator/IpIdentityValidator.java	0.00%	3 Missing and 2 partials ⚠️
...e/core/spannormalizer/rawspan/ByPassPredicate.java	28.57%	2 Missing and 3 partials ⚠️
.../accessor/entities/TraceEntityAccessorBuilder.java	0.00%	4 Missing ⚠️
.../accessor/entities/DefaultTraceEntityAccessor.java	93.02%	1 Missing and 2 partials ⚠️
...wspansgrouper/validator/PeerIdentityValidator.java	40.00%	2 Missing and 1 partial ⚠️
...nrichedspan/constants/utils/EnrichedSpanUtils.java	66.66%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #431      +/-   ##
============================================
- Coverage     79.88%   79.53%   -0.36%     
- Complexity     1419     1427       +8     
============================================
  Files           128      130       +2     
  Lines          5569     5692     +123     
  Branches        512      533      +21     
============================================
+ Hits           4449     4527      +78     
- Misses          884      912      +28     
- Partials        236      253      +17

Flag	Coverage Δ
unit	`79.53% <72.75%> (-0.36%)`	⬇️

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.

aman-bansal · 2024-01-22T08:20:35Z

@@ -0,0 +1,6 @@
+@namespace("org.hypertrace.core.spannormalizer")
+protocol SpanMetadataProtocol {


move it to raw-spans-grouper

There is plan to merge grouper and normaliser, so keeping it together for now.

aman-bansal · 2024-01-22T08:20:45Z

@@ -0,0 +1,15 @@
+@namespace("org.hypertrace.core.spannormalizer")
+protocol PeerIdentityProtocol {


same move to raw spans grouper

aman-bansal · 2024-01-22T08:26:26Z

+            HexUtils.getHex(event.getEventId()),
+            event.getServiceName());
+
+        event


i think we should add this in enrichedAttributes not attribute map. @skjindal93 I believe enriched attributes are for anything that traceable resolves and add to the span

adding to enriched attributes since we changed the attribute name to PEER_SERVICE_NAME

aman-bansal · 2024-01-22T08:27:40Z

+    }
+
+    String agentType =
+        attributeMap


do we have access to EnrichedSpanUtils here? it generally has all this functions

but that is enriched attribute right? here we will not have the enriched attributes.

Using the SpanAttributeUtils.getStringAttributeWithDefault()

kotharironak · 2024-01-29T01:43:24Z

  }
  implementation(project(":span-normalizer:span-normalizer-api"))
+  implementation(project(":semantic-convention-utils"))
+  implementation(project(":hypertrace-trace-enricher:enriched-span-constants"))


Looks like this changes should be part of Trace-Enricher.

We introduced new Attrs in EnrichedSpanConstants and populating them here. So, added dependency

kotharironak · 2024-01-29T02:14:43Z

+    Timestamps timestamps =
+        traceLatencyMeter.trackEndToEndLatencyTimestamps(
+            currentTimeMs, firstEntry ? currentTimeMs : traceState.getTraceStartTimestamp());
+    StructuredTrace trace =


We build structureTrace in two places - 1) TraceEmitPuncutator 2) Bypass condition.

This seems a third place. What is the condition for this - it's not very clear. Will the same trace be also emitted via TraceEmitPuncuator.

No, the same trace be also emitted via TraceEmitPuncuator as we return in the caller function:

hypertrace-ingester/raw-spans-grouper/raw-spans-grouper/src/main/java/org/hypertrace/core/rawspansgrouper/RawSpansProcessor.java

Line 202 in aed68f2

return;

Added a comment

kotharironak · 2024-01-29T02:28:17Z

+trace.emit.punctuator.frequency = 15s
+
+peer.correlation = {
+  enabled.customers = ["*"]


add a separate var: enable.all.customers = true or false

This is the test app.conf, so added * in config. In the actual app.conf it is empty. What is the need of enable.all.customers, we can handle both using the same config.

kotharironak · 2024-01-29T02:30:27Z

@@ -0,0 +1,14 @@
+@namespace("org.hypertrace.core.spannormalizer")


This requires in raw-spans-grouper. Shouldn't this be part of raw-spans-grouper

There were some avros also like: TraceState defined here that were getting used in raw-spans-grouper, so introduced them in a common place. What was the idea behind defining them earlier here? Should we move rest of them also to raw-spans-grouper then?

kotharironak · 2024-01-29T12:50:02Z

    }

    Event event = value.getEvent();
+    // spans whose trace is not created by ByPass flow, their trace will be created in below


Note: For now, it is controlled by customer_id. But, we should check if this is a mirroring agent or singleton trace.

We are checking if its a mirroring agent:

hypertrace-ingester/raw-spans-grouper/raw-spans-grouper/src/main/java/org/hypertrace/core/rawspansgrouper/RawSpansProcessor.java

Line 341 in 0912315

String agentType =

kotharironak · 2024-01-29T12:55:28Z

  @Override
  public boolean test(TraceIdentity traceIdentity, RawSpan rawSpan) {
+    // tenant level spans bypass override
+    if (bypassOverrideTenants.contains(rawSpan.getCustomerId())


bypassExclusionTenants or excludeTenantsFromBypass

kotharironak · 2024-01-29T12:56:00Z

      {{- end }}
+
+      {{- if hasKey .Values.spanNormalizerConfig.processor "bypassOverrideTenants" }}
+      bypass.override.tenants = "{{ .Values.spanNormalizerConfig.processor.bypassOverrideTenants | toJson }}"


bypass.exclusion.tenants.

github-actions · 2024-01-30T05:59:46Z

Unit Test Results

  79 files +1   79 suites +1 1m 7s ⏱️ ±0s
420 tests +2 420 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit fc4d7c0. ± Comparison against base commit b9f458f.

Support service correlation for mirroring spans

95d5ea0

sanket-mundra requested a review from a team as a code owner October 12, 2023 19:31

sanket-mundra requested review from aman-bansal and skjindal93 October 12, 2023 19:31

This comment has been minimized.

Sign in to view

Added test

25bcf21

This comment has been minimized.

Sign in to view

Updated caller to peer

c788574

This comment has been minimized.

Sign in to view

aman-bansal reviewed Oct 16, 2023

View reviewed changes

Addressed review comments

ecb328a

sanket-mundra requested a review from aman-bansal October 30, 2023 14:27

Updates

b4a470e

This comment has been minimized.

Sign in to view

comment

4680b9d

This comment has been minimized.

Sign in to view

add flags

91d57ec

This comment has been minimized.

Sign in to view

aman-bansal reviewed Jan 16, 2024

View reviewed changes

Comment thread span-normalizer/span-normalizer-api/src/main/avro/trace-identity/IpIdentity.avdl Outdated

Addressed review comments

6fadfaf

This comment has been minimized.

Sign in to view

sanket-mundra requested a review from aman-bansal January 16, 2024 12:45

aman-bansal reviewed Jan 22, 2024

View reviewed changes

Comment thread ...raw-spans-grouper/src/main/java/org/hypertrace/core/rawspansgrouper/IpIdentityValidator.java

aman-bansal reviewed Jan 22, 2024

View reviewed changes

Comment thread ...w-spans-grouper/src/main/java/org/hypertrace/core/rawspansgrouper/PeerIdentityValidator.java Outdated

aman-bansal reviewed Jan 22, 2024

View reviewed changes

Addressed review comments

d911dd3

sanket-mundra requested a review from aman-bansal January 22, 2024 11:38

This comment has been minimized.

Sign in to view

updates

4a7b6d8

This comment has been minimized.

Sign in to view

updates

aed68f2

This comment has been minimized.

Sign in to view

sanket-mundra requested a review from kotharironak January 23, 2024 09:48

updates

131029f

This comment has been minimized.

Sign in to view

skjindal93 previously approved these changes Jan 25, 2024

View reviewed changes

kotharironak reviewed Jan 29, 2024

View reviewed changes

Addressed review comments

02cbae2

sanket-mundra dismissed skjindal93’s stale review via 02cbae2 January 29, 2024 09:41

This comment has been minimized.

Sign in to view

update

0912315

This comment has been minimized.

Sign in to view

kotharironak approved these changes Jan 29, 2024

View reviewed changes

Merge branch 'main' into serviceToServiceCorrelation

3a217cf

This comment has been minimized.

Sign in to view

sanket-mundra merged commit fc4d7c0 into main Jan 30, 2024

sanket-mundra deleted the serviceToServiceCorrelation branch January 30, 2024 05:57

		@@ -0,0 +1,6 @@
		@namespace("org.hypertrace.core.spannormalizer")
		protocol SpanMetadataProtocol {

		@@ -0,0 +1,15 @@
		@namespace("org.hypertrace.core.spannormalizer")
		protocol PeerIdentityProtocol {

		@@ -0,0 +1,14 @@
		@namespace("org.hypertrace.core.spannormalizer")

Conversation

sanket-mundra commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Documentation

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

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!

Choose a reason for hiding this comment

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!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Choose a reason for hiding this comment

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!

sanket-mundra commented Oct 12, 2023 •

edited

Loading

codecov Bot commented Oct 12, 2023 •

edited

Loading

sanket-mundra Jan 29, 2024 •

edited

Loading