refactor: consistently use one uuid library by kruskall · Pull Request #40161 · elastic/beats

kruskall · 2024-07-10T02:01:54Z

Proposed commit message

consistently use one uuid library

the codebase is using a mix of github.com/google/uuid and github.com/gofrs/uuid Update the codebase to use github.com/gofrs/uuid consistently and bump to the current major version.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

the codebase is using a mix of github.com/google/uuid and github.com/gofrs/uuid Update the codebase to use github.com/gofrs/uuid consistently and bump to the current major version.

mergify · 2024-07-10T02:02:30Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kruskall? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

efd6 · 2024-07-10T02:29:20Z

Is there a good argument for one over the other? The APIs look pretty similar, but there are some aspects of each that are not in the other, so one is not a drop in replacement for the other.

kruskall · 2024-07-10T03:36:14Z

Good point!

I don't have a strong opinion on either. To be honest, I went with github.com/gofrs/uuid because that's what we are using in https://github.com/elastic/elastic-agent-system-metrics/

The APIs look pretty similar, but there are some aspects of each that are not in the other, so one is not a drop in replacement for the other.

Can you expand on this ?

efd6 · 2024-07-10T03:56:41Z

Can you expand on this ?

The constructor sets differing is the most obvious thing; there is no gofrs/uuid.New.
The errors that are returned are different (I think gofrs is stronger here, but they differ, so use of the error values needs to be checked).
There are a few method set differences, so interface satisfaction will differ which may not be noticeable for dynamically asserted interface behaviour.

belimawr

So far the PR looks great! I don't have any strong preference about which library to use, but I like the fact that github.com/gofrs/uuid is community maintained.

leehinman · 2024-07-10T21:24:07Z

@kruskall do you think it would be worth it to add the google/uuid to the gomodguard section of .golangci.yml file? That way CI can help us stay with one library.

elasticmachine · 2024-08-05T13:51:54Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

elasticmachine · 2024-08-05T13:51:54Z

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

elasticmachine · 2024-08-05T13:51:54Z

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

elasticmachine · 2024-08-05T13:51:54Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

elasticmachine · 2024-08-05T13:51:55Z

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

efd6

It's a pity that this is mixed up with lint clean up.

efd6 · 2024-08-05T21:49:45Z

filebeat/input/log/harvester.go

-			switch err {
-			case ErrFileTruncate:
+			switch {
+			case errors.Is(err, ErrFileTruncate):


I don't see any cases that these error values are returned wrapped so (disclaimer: as a non-owner) I'm not convinced of the value of this, but if this change is going to be made, the tests that look at error values should be updated to match (ErrInactive is the target of an exact value check).

efd6 · 2024-08-05T22:11:27Z

libbeat/monitoring/inputmon/input.go

 	log := logp.NewLogger("metric_registry")
 	// Make an orthogonal ID to allow tracking register/deregister pairs.
-	uuid := uuid.New().String()
+	uuid := uuid.Must(uuid.NewV4()).String()


AFAICS crypto/rand.Reader hides EAGAIN so this should be OK.

kruskall · 2024-08-08T00:31:19Z

@elastic/beats-tech-leads sorry for the ping, this only need one more review. Any help ? 🙏

libbeat/cmd/instance/beat.go

pkoutsovasilis

~~A single nit about a check that might have become redundant, but~~ this LGTM

kruskall added the enhancement label Jul 10, 2024

kruskall requested review from a team as code owners July 10, 2024 02:01

kruskall requested a review from belimawr July 10, 2024 02:01

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jul 10, 2024

kruskall requested a review from fearful-symmetry July 10, 2024 02:01

mergify bot assigned kruskall Jul 10, 2024

kruskall marked this pull request as draft July 10, 2024 02:08

lint: fix linting issues

e8c21b5

belimawr reviewed Jul 10, 2024

View reviewed changes

kruskall mentioned this pull request Jul 10, 2024

refactor: bump github.com/gofrs/uuid to current major version elastic/elastic-agent-libs#219

Merged

3 tasks

kruskall added 8 commits July 11, 2024 01:07

Merge branch 'main' into feat/standard-uuid-lib

aa7304f

refactor: move to gofrs/uuid

0fee800

lint: add gomodguard setting

fb77a5a

lint: fix more linter issues

74beaf9

lint: more linting errors

3e40b83

lint: completely remove gofrs/uuid v4

7772b15

lint: more linting errors

f500379

lint: more linting errors

ec7de41

kruskall added 3 commits August 1, 2024 01:45

Merge branch 'main' into feat/standard-uuid-lib

e4cdd74

Merge branch 'main' into feat/standard-uuid-lib

e7566f1

test: gracefully log the error

4c5cf4c

kruskall marked this pull request as ready for review August 1, 2024 23:55

kruskall added 2 commits August 2, 2024 17:49

Merge branch 'main' into feat/standard-uuid-lib

cad2258

Merge branch 'main' into feat/standard-uuid-lib

285779f

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 5, 2024

nfritts approved these changes Aug 5, 2024

View reviewed changes

efd6 approved these changes Aug 5, 2024

View reviewed changes

pkoutsovasilis reviewed Aug 12, 2024

View reviewed changes

libbeat/cmd/instance/beat.go Show resolved Hide resolved

pkoutsovasilis approved these changes Aug 12, 2024

View reviewed changes

andrewkroh approved these changes Aug 29, 2024

View reviewed changes

leehinman approved these changes Sep 6, 2024

View reviewed changes

axw approved these changes Sep 9, 2024

View reviewed changes

kaiyan-sheng approved these changes Sep 9, 2024

View reviewed changes

kruskall merged commit cd51941 into elastic:main Sep 9, 2024

kruskall deleted the feat/standard-uuid-lib branch September 9, 2024 15:42

andrewkroh mentioned this pull request Sep 10, 2024

[Journald] Restart journalctl if it exits unexpectedly #40558

Merged

6 tasks

Conversation

kruskall commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

mergify bot commented Jul 10, 2024

Uh oh!

efd6 commented Jul 10, 2024

Uh oh!

kruskall commented Jul 10, 2024

Uh oh!

efd6 commented Jul 10, 2024

Uh oh!

belimawr left a comment

Choose a reason for hiding this comment

Uh oh!

leehinman commented Jul 10, 2024

Uh oh!

elasticmachine commented Aug 5, 2024

Uh oh!

elasticmachine commented Aug 5, 2024

Uh oh!

elasticmachine commented Aug 5, 2024

Uh oh!

elasticmachine commented Aug 5, 2024

Uh oh!

elasticmachine commented Aug 5, 2024

Uh oh!

efd6 left a comment

Choose a reason for hiding this comment

Uh oh!

efd6 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

efd6 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

kruskall commented Aug 8, 2024

Uh oh!

Uh oh!

pkoutsovasilis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

kruskall commented Jul 10, 2024 •

edited

Loading

pkoutsovasilis left a comment •

edited

Loading