Reporter: allow extending samples with extra attributes #237

athre0z · 2024-11-12T16:43:29Z

This PR does two things:

Replace `addProfileAttributes` and `attrKeyValue[T]` with `AttrTableManager`

I found the previous design with the generic on addProfileAttributes rather inelegant. Firstly, there's no covariance on Go generics, so it would take two addProfileAttributes calls and an append to add both string and int attributes:

opentelemetry-ebpf-profiler/reporter/otlp_reporter.go

Lines 669 to 675 in 47e8410

    
           sample.Attributes = append(addProfileAttributes(profile, []attrKeyValue[string]{ 
        
           	{key: string(semconv.ContainerIDKey), value: traceKey.containerID}, 
        
           	{key: string(semconv.ThreadNameKey), value: traceKey.comm}, 
        
           	{key: string(semconv.ServiceNameKey), value: traceKey.apmServiceName}, 
        
           }, attributeMap), addProfileAttributes(profile, []attrKeyValue[int64]{ 
        
           	{key: string(semconv.ProcessPIDKey), value: traceKey.pid}, 
        
           }, attributeMap)...)

It also required reflection in addProfileAttributes for no good reason. IMHO this isn't really a good use-case for generics. In AttrTableManager this is replaced with two separate methods that each add just one attribute.

Secondly, a proper type for this results in a much cleaner interface for the second change discussed below.

Introduce `SampleAttrProducer` interface

This allows agent implementations to add custom sample attributes without maintaining a fork of the reporter implementation. It can gather extra meta-data when the trace is initially added and then emit extra attributes when the sample is sent out.

athre0z · 2024-11-12T17:43:28Z

reporter/attrmgr_test.go

 		expectedIndices        [][]uint64
 		expectedAttributeTable []*common.KeyValue
 	}{
 		"empty": {
-			profile: &profiles.Profile{},


Not sure why this was in the struct: it seems that simply making this a local temporary variable in the loop below is easier. I reworked it accordingly, but perhaps I'm missing something on why this was done like this originally.

athre0z · 2024-11-12T17:43:35Z

reporter/attrmgr_test.go

@@ -29,7 +27,6 @@ func TestGetSampleAttributes(t *testing.T) {
 					pid:            0,
 				},
 			},
-			attributeMap:    make(map[string]uint64),


rockdaboot · 2024-11-12T18:11:50Z

Overall looks better than the generics approach 👍

rockdaboot · 2024-11-13T08:21:32Z

reporter/attrmgr.go

+	return m.addAnyAttr(key, compound, &val)
+}
+
+func (m *AttrTableManager) AddStringAttr(key, value string) AttrIndex {


How would skipping empty strings work here (as suggested in #233)? We can not just return a 0 index.

I was under the impression that the discussion in #233 was leaning towards not doing the filtering in the function/component responsible for managing the attribute table. In the interest of not spreading the discussion over two issues, I'll let this PR wait until #233 is merged. If we decide on letting the manager fn filter, I can simply rework this to:

AppendStringAttr(attrs []AttrIndex, key, value string) []AttrIndex

florianl · 2024-11-13T08:15:53Z

reporter/config.go

+// - inspect each trace and its meta when it is enqueued in the reporter
+// - produce extra meta info
+// - attach extra attributes to the trace
+type SampleAttrProducer interface {


Should this be moved to iface.go?

I considered that originally, but iface.go is currently dedicated to the public interface implemented by any reporter. If found config.go to be a better fit. I didn't want readers to incorrectly conclude that they have to implement this for a reporter.

florianl · 2024-11-13T08:19:13Z

reporter/attrmgr.go

+	profile *profiles.Profile
+}
+
+func NewAttrTableManager(profile *profiles.Profile) *AttrTableManager {


*profiles.Profile is internal to the package reporter. How are users of the package supposed to create it?
How should the integration look like, for a different kind of reporter, like for the OTel collector? see #208

From the updated testcase (TestGetSampleAttributes), where this API is used, it looks more like this API builds a parallel *profiles.Profile on top of an existing one just with attributes but without traces.

How is this private? It's a public type from the OTEL proto lib?

From the updated testcase (TestGetSampleAttributes), where this API is used, it looks more like this API builds a parallel *profiles.Profile on top of an existing one just with attributes but without traces.

No, it's just populating the attribute table portion. I'll apply the change that Tim suggested here to make this clearer.

Ah, I looked through #208 now, and I think what you are referring to is that it will be private in the future, replaced by the pdata type? I hadn't followed up with the collector reporter impl -- still catching up after ~1 month mostly away from this repo here -- apologies! Fortunately the changes that Tim suggested (and that I have applied now) should allow the type to generalize to allow using it in the new reporter implementation as well.

florianl · 2024-11-13T08:21:24Z

reporter/attrmgr.go

+	return m.addAnyAttr(key, compound, &val)
+}
+
+func (m *AttrTableManager) AddStringAttr(key, value string) AttrIndex {


This opens the discussion again around sending empty values for attributes that are not required - see #233

reporter/attrmgr.go

Co-authored-by: Tim Rühsen <[email protected]>

Rresolved conflict with profiling vs htlhash key

athre0z · 2024-11-13T11:28:24Z

reporter/attrmgr.go

+	attrTable *[]*common.KeyValue
+}
+
+func NewAttrTableManager(attrTable *[]*common.KeyValue) *AttrTableManager {


Alternatively we could not take that any argument, and rename the type to AttrTableBuilder. In that case I'd then add a new method GetAttrTable() that returns the final built table. That'd change the workflow for the caller to:

Create empty instance

Populate it with AddXXX calls

Take the final table with GetAttrTable() and manually assign it to the attribute table field

Perhaps more obvious than mutating the slice field in place through a pointer? No strong preference here.

I thought about this as well (named it Get() to not repeat AttrTable), but it comes with the downside that you need to call this function when done. And this can be forgotten, while the solution in this PR is safer from accidental failure.

My not very strong preference would be to avoid an extra Get function call.

rockdaboot · 2024-11-13T11:57:17Z

reporter/otlp_reporter.go

-						{key: "process.executable.build_id.htlhash",
-							value: traceInfo.files[i].StringNoQuotes()},
-					}, attributeMap)
+						attrMgr.AddStringAttr("process.executable.build_id.gnu",


nit: Rename AddStringAttr to AddString to avoid repetition of "Attr". Same for AddIntAttr.

athre0z added 2 commits November 12, 2024 17:12

Reporter: add attribute manager

00e4772

Reporter: allow adding custom sample attributes

cdb7be3

athre0z added the enhancement New feature or request label Nov 12, 2024

fixup: happy linter

d49e00e

athre0z self-assigned this Nov 12, 2024

athre0z added 2 commits November 12, 2024 18:44

fixup: remove another unused struct field

96ce376

fixup: add back lost comment

fdb8cc6

athre0z commented Nov 12, 2024

View reviewed changes

athre0z marked this pull request as ready for review November 12, 2024 18:24

athre0z requested review from a team as code owners November 12, 2024 18:24

rockdaboot reviewed Nov 13, 2024

View reviewed changes

florianl reviewed Nov 13, 2024

View reviewed changes

rockdaboot reviewed Nov 13, 2024

View reviewed changes

reporter/attrmgr.go Outdated Show resolved Hide resolved

reporter/attrmgr.go Outdated Show resolved Hide resolved

rockdaboot reviewed Nov 13, 2024

View reviewed changes

reporter/attrmgr.go Show resolved Hide resolved

rockdaboot reviewed Nov 13, 2024

View reviewed changes

reporter/attrmgr.go Outdated Show resolved Hide resolved

athre0z and others added 4 commits November 13, 2024 11:53

fixup: add vanity import

64a269b

Co-authored-by: Tim Rühsen <[email protected]>

fixup: generalize manager away from taking a whole profile

f7a7fc1

fixup: use attribute.Key

d0a6a5b

Merge branch 'main' into joel/custom-sample-attrs

46079c0

Rresolved conflict with profiling vs htlhash key

athre0z commented Nov 13, 2024

View reviewed changes

rockdaboot reviewed Nov 13, 2024

View reviewed changes

fixup: fix breakage after merge

d0f2d98

rockdaboot approved these changes Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reporter: allow extending samples with extra attributes #237

Reporter: allow extending samples with extra attributes #237

athre0z commented Nov 12, 2024 •

edited

Loading

athre0z Nov 12, 2024

athre0z Nov 12, 2024

rockdaboot commented Nov 12, 2024

rockdaboot Nov 13, 2024

athre0z Nov 13, 2024 •

edited

Loading

florianl Nov 13, 2024

athre0z Nov 13, 2024

florianl Nov 13, 2024 •

edited

Loading

athre0z Nov 13, 2024

athre0z Nov 13, 2024

florianl Nov 13, 2024

athre0z Nov 13, 2024

rockdaboot Nov 13, 2024 •

edited

Loading

rockdaboot Nov 13, 2024

	sample.Attributes = append(addProfileAttributes(profile, []attrKeyValue[string]{
	{key: string(semconv.ContainerIDKey), value: traceKey.containerID},
	{key: string(semconv.ThreadNameKey), value: traceKey.comm},
	{key: string(semconv.ServiceNameKey), value: traceKey.apmServiceName},
	}, attributeMap), addProfileAttributes(profile, []attrKeyValue[int64]{
	{key: string(semconv.ProcessPIDKey), value: traceKey.pid},
	}, attributeMap)...)

Reporter: allow extending samples with extra attributes #237

Are you sure you want to change the base?

Reporter: allow extending samples with extra attributes #237

Conversation

athre0z commented Nov 12, 2024 • edited Loading

Replace addProfileAttributes and attrKeyValue[T] with AttrTableManager

Introduce SampleAttrProducer interface

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockdaboot commented Nov 12, 2024

Choose a reason for hiding this comment

athre0z Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianl Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockdaboot Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athre0z commented Nov 12, 2024 •

edited

Loading

Replace `addProfileAttributes` and `attrKeyValue[T]` with `AttrTableManager`

Introduce `SampleAttrProducer` interface

athre0z Nov 13, 2024 •

edited

Loading

florianl Nov 13, 2024 •

edited

Loading

rockdaboot Nov 13, 2024 •

edited

Loading