[component] Make InstanceID immutable #10495

mwear · 2024-07-01T23:07:57Z

Description

This PR makes component.InstanceID immutable. Previously it was a struct with all fields exported. Technically this is a breaking change, but the only thing using the InstanceID is the in-progress healthcheckv2extension.

Link to tracking issue

Fixes #10494

Testing

units

Documentation

code comments

codecov · 2024-07-01T23:37:28Z

Codecov Report

Attention: Patch coverage is 93.10345% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.67%. Comparing base (fce1d46) to head (83dcd9d).
Report is 13 commits behind head on main.

Files	Patch %	Lines
component/componentstatus/instance.go	89.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10495      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.02%     
==========================================
  Files         404      405       +1     
  Lines       18947    18971      +24     
==========================================
+ Hits        17371    17391      +20     
- Misses       1217     1221       +4     
  Partials      359      359

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

component/component.go

mx-psi

This LGTM. I am happy to merge this if we have a link to an issue on grafana/alloy and grafana/agent about this change

mwear · 2024-07-17T17:20:45Z

I'll get on those issues and bring this out of draft and rebase.

mwear · 2024-07-17T17:48:43Z

Issues have been opened in the respective grafana repos:

axw

Just a couple of drive-by comments, feel free to ignore :)

axw · 2024-07-18T00:54:05Z

component/component.go

+func (id *InstanceID) PipelineIDs() map[ID]struct{} {
+	return id.pipelineIDs
+}


Maybe it's over the top, but did you consider returning a copy of the map, to ensure the ID is truly immutable? Otherwise a caller can still modify the object.

I also wonder if it would be sensible to store the pipeline IDs in a way that makes the InstanceID type comparable. Again it doesn't seem to matter at the moment, since InstanceID is passed around by reference everywhere. Just thinking it could be good for future proofing.

I had considered returning a copy of the map, but I wasn't sure it was worth the performance hit. These will be read often. This is an easy change to make though if anyone has strong opinions. Do you have a suggestion for how we might store PipelineIDs in a way that will make InstanceID comparable? If so, I'd be open to considering it. Bonus if it solves the true immutability problem as well.

I had considered returning a copy of the map, but I wasn't sure it was worth the performance hit. These will be read often. This is an easy change to make though if anyone has strong opinions.

Fair enough. I don't have a very strong opinion on this, just stumbled on the PR and thought it would be worth checking.

Do you have a suggestion for how we might store PipelineIDs in a way that will make InstanceID comparable? If so, I'd be open to considering it. Bonus if it solves the true immutability problem as well.

One option would be to:

marshal the IDs, with some separator (space?) that's not permitted in type or name, and store as a string

change PipelineIDs to an iterator-type function like pcommon.Map.Range which incrementally unmarshals the IDs

The range function could be written in a way that works with Go 1.23's upcoming iterator functions: https://pkg.go.dev/iter#hdr-Iterators

Both of these properties seem to me like things that could be added in the future in a backwards compatible way. If anything, we can document now that this is not comparable and that the map should not be modified, and leave adding this extra guarantees for a future PR.

Seems fine to me. Having map[ID]struct{} in the exposed API doesn't seem ideal, and making it an iterator would be breaking again, but I don't think it's worth blocking over.

making it an iterator would be breaking again, but I don't think it's worth blocking over.

I don't feel like it's incompatible to add a new method for the iterator if we ensure the data is presented in the same way. I'll wait for Matt to give his opinion as well :)

@axw, I refactored the InstanceID to be comparable per your suggestions. Take a look and see if this is what you had in mind. Let me know if it needs any further work. I chose the second (or third) best name for the iterator to leave the door open to rename it (to AllPipelineIDs or just PipelineIDs) when we can use the iterator functions coming in Go 1.23.

axw

@mwear looks great, and exactly what I meant, but I just tested locally and found that IDs can have spaces in them. I'd be inclined to revert to using a map internally, and follow up on making it comparable later as @mx-psi suggested. Sorry for the diversion.

axw · 2024-07-19T00:47:48Z

component/component.go

-// InstanceID uniquely identifies a component instance
+// pipelineDelim is the delimeter for internal representation of pipeline
+// component IDs.
+const pipelineDelim = byte(0x20)


Unfortunately it turns out that IDs can contain a space :(

I mean I can define a receiver like this:

receivers: "otlp/abc def": protocols: grpc: endpoint: localhost:4317 http: endpoint: localhost:4318

Even a null byte in the name is accepted.

Seems a little lax to me. Perhaps revert to a map for now, but keep the method change, and we could follow up on making the type comparable later after finding a way to safely encode the IDs? Could either use size prefixing (non-breaking), or add some restrictions to IDs (breaking, but maybe a good idea anyway).

We already introduced restrictions on the part before the / and had no user reports of it breaking them, so I would be in favor of doing the same for the name part (after the /)

I can take a look at adding the same restrictions to the name as we currently have on the type. If it's not terrible, maybe we can make both changes? If it turns out to be more work than expected, we can keep the iterator, and use a map internally, then make InstanceID comparable when we have restrictions on the name.

Yeah that makes sense to me

I opened #10674 to handle restrictions to the name portion of a component.ID. The needs were slightly different so I introduced a different type for the name field.

#### Description While working on #10495 we discovered that there are not any restrictions on the `name` field of a `component.ID`. There are restrictions on the `type` field introduced in #9208. This PR adds similar restrictions to `name`. A type must - have at least one character, - start with an ASCII alphabetic character and - can only contain ASCII alphanumeric characters and '_'. I found that we need a slightly different set of rules for name as some tests use a digit and others use a uuid as a name. A name is still optional, but if it's provided it must: - have at least one character, - start with an ASCII alphanumeric character and - can only contain ASCII alphanumeric characters, '_', and '-'. I'd be willing to adjust these restrictions if anyone has any opinions on what should or should not be allowed.  #### Link to tracking issue Fixes #10673  #### Testing Unit tests  #### Documentation Code comments

mx-psi · 2024-07-31T17:15:36Z

Can this move forward now that #10674 is merged?

mwear · 2024-07-31T21:27:18Z

Can this move forward now that #10674 is merged?

This can move forward, but now that #10730 has merged, I'm wondering if I should update componentstatus.InstanceID instead and what the order of operations should be given #10777.

mx-psi · 2024-08-01T11:12:53Z

Good point, we can wait for #10777

github-actions · 2024-08-16T03:16:31Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

mx-psi · 2024-08-16T07:27:21Z

This is now unblocked since I merged #10777

component/componentstatus/instance.go

atoulme

Changes look good to me, one nit. I didn't check why CI is failing on contrib.

Co-authored-by: Antoine Toulme <[email protected]>

mwear · 2024-08-16T19:10:09Z

Changes look good to me, one nit. I didn't check why CI is failing on contrib.

This is expected to temporarily break contrib. It's a breaking change to the newly introduced componentstatus.InstanceID. This PR originally targeted component.InstanceID, put was put on hold to accommodate the component status refactor. I'll fix contrib when this merges.

.chloggen/immutable-instance-id.yaml

component/componentstatus/instance.go

Co-authored-by: Tyler Helmuth <[email protected]>

.chloggen/immutable-instance-id.yaml

Co-authored-by: Tyler Helmuth <[email protected]>

TylerHelmuth · 2024-08-20T15:03:42Z

@mwear please take a look at the failing lint

mwear mentioned this pull request Jul 1, 2024

[component] Make InstanceID immutable #10494

Closed

mwear force-pushed the immutable-instance-id branch from bbfba13 to 26d02fb Compare July 1, 2024 23:29

mx-psi reviewed Jul 5, 2024

View reviewed changes

component/component.go Outdated Show resolved Hide resolved

component/component.go Outdated Show resolved Hide resolved

mwear force-pushed the immutable-instance-id branch from 26d02fb to e9004ae Compare July 8, 2024 18:32

mx-psi approved these changes Jul 17, 2024

View reviewed changes

mwear force-pushed the immutable-instance-id branch from e9004ae to 18dccc3 Compare July 17, 2024 17:26

mwear marked this pull request as ready for review July 17, 2024 17:27

mwear requested review from a team and songy23 July 17, 2024 17:27

This was referenced Jul 17, 2024

Breaking changes to component.InstanceID grafana/agent#6984

Open

Breaking changes to component.InstanceID grafana/alloy#1323

Closed

axw reviewed Jul 18, 2024

View reviewed changes

mwear force-pushed the immutable-instance-id branch 2 times, most recently from 5e7d9eb to 214dc11 Compare July 18, 2024 23:33

axw reviewed Jul 19, 2024

View reviewed changes

This was referenced Jul 19, 2024

[component] Restrict allowed characters for component.ID name #10673

Closed

[component] Restrict character set for component.ID name #10674

Merged

mwear mentioned this pull request Jul 29, 2024

[component] Move component status reporting public API to new componentstatus module #10730

Merged

github-actions bot added the Stale label Aug 16, 2024

mx-psi removed the Stale label Aug 16, 2024

Make componentstatus.InstanceID immutable and comparable

c8d3033

mwear force-pushed the immutable-instance-id branch from 214dc11 to c8d3033 Compare August 16, 2024 16:42

atoulme reviewed Aug 16, 2024

View reviewed changes

component/componentstatus/instance.go Outdated Show resolved Hide resolved

atoulme approved these changes Aug 16, 2024

View reviewed changes

Update component/componentstatus/instance.go

6d9df8c

Co-authored-by: Antoine Toulme <[email protected]>

TylerHelmuth reviewed Aug 16, 2024

View reviewed changes

.chloggen/immutable-instance-id.yaml Outdated Show resolved Hide resolved

component/componentstatus/instance.go Outdated Show resolved Hide resolved

mwear and others added 2 commits August 16, 2024 14:27

Update .chloggen/immutable-instance-id.yaml

ccccd71

Co-authored-by: Tyler Helmuth <[email protected]>

Move comment

ba1e6bf

TylerHelmuth approved these changes Aug 16, 2024

View reviewed changes

.chloggen/immutable-instance-id.yaml Outdated Show resolved Hide resolved

mwear and others added 2 commits August 16, 2024 14:31

Update .chloggen/immutable-instance-id.yaml

84a220a

Co-authored-by: Tyler Helmuth <[email protected]>

Merge branch 'main' into immutable-instance-id

0718613

TylerHelmuth added the Skip Contrib Tests label Aug 20, 2024

mwear and others added 2 commits August 20, 2024 08:18

Lint

2a4c9af

Merge branch 'main' into immutable-instance-id

83dcd9d

mx-psi approved these changes Aug 20, 2024

View reviewed changes

songy23 approved these changes Aug 20, 2024

View reviewed changes

mx-psi merged commit 98fb888 into open-telemetry:main Aug 21, 2024
35 of 36 checks passed

github-actions bot added this to the next release milestone Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[component] Make InstanceID immutable #10495

[component] Make InstanceID immutable #10495

mwear commented Jul 1, 2024

codecov bot commented Jul 1, 2024 •

edited

Loading

mx-psi left a comment

mwear commented Jul 17, 2024

mwear commented Jul 17, 2024

axw left a comment

axw Jul 18, 2024

mwear Jul 18, 2024

axw Jul 18, 2024

mx-psi Jul 18, 2024

axw Jul 18, 2024

mx-psi Jul 18, 2024

mwear Jul 18, 2024

axw left a comment

axw Jul 19, 2024

mx-psi Jul 19, 2024

mwear Jul 19, 2024

mx-psi Jul 19, 2024

mwear Jul 20, 2024

mx-psi commented Jul 31, 2024 •

edited

Loading

mwear commented Jul 31, 2024 •

edited

Loading

mx-psi commented Aug 1, 2024

github-actions bot commented Aug 16, 2024

mx-psi commented Aug 16, 2024

atoulme left a comment

mwear commented Aug 16, 2024

TylerHelmuth commented Aug 20, 2024

[component] Make InstanceID immutable #10495

[component] Make InstanceID immutable #10495

Conversation

mwear commented Jul 1, 2024

Description

Link to tracking issue

Testing

Documentation

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

mx-psi left a comment

Choose a reason for hiding this comment

mwear commented Jul 17, 2024

mwear commented Jul 17, 2024

axw left a 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

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

axw left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mx-psi commented Jul 31, 2024 • edited Loading

mwear commented Jul 31, 2024 • edited Loading

mx-psi commented Aug 1, 2024

github-actions bot commented Aug 16, 2024

mx-psi commented Aug 16, 2024

atoulme left a comment

Choose a reason for hiding this comment

mwear commented Aug 16, 2024

TylerHelmuth commented Aug 20, 2024

codecov bot commented Jul 1, 2024 •

edited

Loading

mx-psi commented Jul 31, 2024 •

edited

Loading

mwear commented Jul 31, 2024 •

edited

Loading