feat: replace custom atomic with sync/atomic #40132

kruskall · 2024-07-08T01:19:21Z

Proposed commit message

Go 1.19 added new atomic types. Replace custom atomic types with stdlib atomic package to fully use the race detector, improved lsp hints and support.

Use pointers in sniffer struct to prevent copying the atomic struct and cause a race condition.

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

Go 1.19 added new atomic types. Replace custom atomic types with stdlib atomic package to fully use the race detector, improved lsp hints and support. Use pointers in sniffer struct to prevent copying the atomic struct and cause a race condition.

mergify · 2024-07-08T01:19:58Z

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

fix golangci-lint issue

kruskall · 2024-07-08T01:44:36Z

import 'github.com/elastic/beats/v7/x-pack/dockerlogbeat/pipelinemock' is not allowed from list 'apache-licensed-code': Apache 2.0 licensed code cannot depend on Elastic licensed code (x-pack/). (depguard)

This doesn't look like an issue with this PR 🤔

Is it intended ?

filebeat/channel/outlet.go

heartbeat/scheduler/schedjob_test.go

libbeat/management/agent.go

efd6 · 2024-07-08T02:06:57Z

libbeat/outputs/elasticsearch/client_proxy_test.go

+	_serverRequestCount atomic.Int64 // Requests directly to the server
+	_proxyRequestCount  atomic.Int64 // Requests via the proxy


The original is 32 bits wide. Do we care? There is almost no-one in the world who that would affect, and it would have affected them anyway. Raising only for awareness.

libbeat/publisher/pipeline/stress/sig.go

libbeat/tests/integration/framework.go

metricbeat/mb/module/runner_group_test.go

efd6 · 2024-07-08T02:31:33Z

packetbeat/sniffer/sniffer.go

 	config config.InterfaceConfig

-	state atomic.Int32 // store snifferState
+	state *atomic.Int32 // store snifferState


Rather than making this a pointer, the child could be directly assigned to sniffers[i] at L105 and child be made a *sniffer from sniffers[i] for ergonomics immediately after. Your choice.

winlogbeat/sys/wineventlog/renderer_test.go

Co-authored-by: Dan Kortschak <[email protected]>

fix golangci-lint issue

cmacknz · 2024-07-17T20:16:51Z

github.com/elastic/beats/v7/x-pack/dockerlogbeat/pipelinemock

We can just Apache license the package most likely and move it out of x-pack (or get rid of our use of this entirely).

andrewkroh · 2024-07-17T20:20:47Z

@pkoutsovasilis Here's an issue for that. #40293

leehinman

One question on removal of closeOnce, otherwise I'm OK with this.

libbeat/publisher/pipeline/client.go

andrzej-stencel · 2024-08-13T13:00:47Z

I think we should create an issue to handle this from the Linux linter

filebeat/input/filestream/internal/input-logfile/harvester_test.go:36:2: import 'github.com/elastic/beats/v7/x-pack/dockerlogbeat/pipelinemock' is not allowed from list 'apache-licensed-code': Apache 2.0 licensed code cannot depend on Elastic licensed code (x-pack/). (depguard)

This PR is blocked by the linter failure, as reported in this issue:

OSS filebeat harvester_test.go should not depend on Elastic-licensed code #40293

There's a PR up to fix that issue:

Remove x-pack code from OSS filebeat. #40483

@leehinman perhaps you could take a look at that PR to unblock this one?

ycombinator · 2024-09-03T23:43:53Z

#40483 has now been merged, so this PR here should now be unblocked.

mergify · 2024-09-21T13:48:08Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

kruskall · 2024-11-21T17:57:15Z

@elastic/obs-infraobs-integrations @elastic/obs-ds-hosted-services @elastic/obs-cloud-monitoring @elastic/sec-windows-platform friendly ping for review

andrewkroh

Approving changes to winlogbeat/* on behalf of sec-windows-platform.

kruskall · 2024-12-09T22:32:26Z

@elastic/obs-cloud-monitoring @elastic/obs-ds-hosted-services friendly ping 🙂

vigneshshanmugam

Thanks @kruskall Heartbeat changes LGTM

* feat: replace custom atomic with sync/atomic Go 1.19 added new atomic types. Replace custom atomic types with stdlib atomic package to fully use the race detector, improved lsp hints and support. Use pointers in sniffer struct to prevent copying the atomic struct and cause a race condition. * lint: time.Now().Sub -> time.Since fix golangci-lint issue * Apply suggestions from code review Co-authored-by: Dan Kortschak <[email protected]> * refactor: keep Swap usage as is * lint: avoid unnecessary conversion fix golangci-lint issue * fix: update method reference to new atomic types * fix: resolve more compile errors and test failures * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: more linting errors * lint: fix linter issues * lint: remove unused import * lint: fix linter issue * lint: fix linter issues * lint: fix linter issues --------- Co-authored-by: Dan Kortschak <[email protected]> (cherry picked from commit 296b83b) # Conflicts: # filebeat/inputsource/common/streaming/listener.go

* feat: replace custom atomic with sync/atomic Go 1.19 added new atomic types. Replace custom atomic types with stdlib atomic package to fully use the race detector, improved lsp hints and support. Use pointers in sniffer struct to prevent copying the atomic struct and cause a race condition. * lint: time.Now().Sub -> time.Since fix golangci-lint issue * Apply suggestions from code review Co-authored-by: Dan Kortschak <[email protected]> * refactor: keep Swap usage as is * lint: avoid unnecessary conversion fix golangci-lint issue * fix: update method reference to new atomic types * fix: resolve more compile errors and test failures * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: more linting errors * lint: fix linter issues * lint: remove unused import * lint: fix linter issue * lint: fix linter issues * lint: fix linter issues --------- Co-authored-by: Dan Kortschak <[email protected]>

…41979) * feat: replace custom atomic with sync/atomic (#40132) * feat: replace custom atomic with sync/atomic Go 1.19 added new atomic types. Replace custom atomic types with stdlib atomic package to fully use the race detector, improved lsp hints and support. Use pointers in sniffer struct to prevent copying the atomic struct and cause a race condition. * lint: time.Now().Sub -> time.Since fix golangci-lint issue * Apply suggestions from code review Co-authored-by: Dan Kortschak <[email protected]> * refactor: keep Swap usage as is * lint: avoid unnecessary conversion fix golangci-lint issue * fix: update method reference to new atomic types * fix: resolve more compile errors and test failures * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: fix linter issues * lint: more linting errors * lint: fix linter issues * lint: remove unused import * lint: fix linter issue * lint: fix linter issues * lint: fix linter issues --------- Co-authored-by: Dan Kortschak <[email protected]> (cherry picked from commit 296b83b) # Conflicts: # filebeat/inputsource/common/streaming/listener.go * Update listener.go --------- Co-authored-by: kruskall <[email protected]>

kruskall added the enhancement label Jul 8, 2024

kruskall requested review from a team as code owners July 8, 2024 01:19

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

kruskall requested review from andrzej-stencel, faec and leehinman July 8, 2024 01:19

mergify bot assigned kruskall Jul 8, 2024

lint: time.Now().Sub -> time.Since

d9663fc

fix golangci-lint issue

efd6 reviewed Jul 8, 2024

View reviewed changes

kruskall and others added 6 commits July 8, 2024 04:39

Apply suggestions from code review

d99e0b0

Co-authored-by: Dan Kortschak <[email protected]>

refactor: keep Swap usage as is

7e42bff

lint: avoid unnecessary conversion

96a8196

fix golangci-lint issue

Merge branch 'main' into feat/drop-custom-atomic

aaf5f9d

Merge branch 'main' into feat/drop-custom-atomic

b5aee93

fix: update method reference to new atomic types

2429c76

pkoutsovasilis approved these changes Jul 17, 2024

View reviewed changes

andrewkroh mentioned this pull request Jul 17, 2024

OSS filebeat harvester_test.go should not depend on Elastic-licensed code #40293

Closed

leehinman reviewed Aug 5, 2024

View reviewed changes

libbeat/publisher/pipeline/client.go Show resolved Hide resolved

leehinman approved these changes Aug 5, 2024

View reviewed changes

kruskall added 3 commits September 4, 2024 01:49

Merge branch 'main' into feat/drop-custom-atomic

7289048

lint: remove unused import

64e3f5c

lint: fix linter issue

4d28718

andrzej-stencel approved these changes Sep 4, 2024

View reviewed changes

Merge branch 'main' into feat/drop-custom-atomic

b096ffb

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 21, 2024

kruskall mentioned this pull request Sep 28, 2024

fix(auditbeat): use correct filetimes field names in some systems #41038

Merged

6 tasks

kruskall added 3 commits October 7, 2024 02:03

Merge branch 'main' into feat/drop-custom-atomic

d619516

lint: fix linter issues

63f0221

lint: fix linter issues

485a6d4

shmsr approved these changes Nov 21, 2024

View reviewed changes

andrewkroh approved these changes Dec 6, 2024

View reviewed changes

vigneshshanmugam approved these changes Dec 10, 2024

View reviewed changes

kruskall merged commit 296b83b into elastic:main Dec 10, 2024
2 checks passed

kruskall deleted the feat/drop-custom-atomic branch December 10, 2024 22:08

mergify bot mentioned this pull request Dec 10, 2024

[8.x](backport #40132) feat: replace custom atomic with sync/atomic #41979

Merged

6 tasks

		_serverRequestCount atomic.Int64 // Requests directly to the server
		_proxyRequestCount atomic.Int64 // Requests via the proxy

feat: replace custom atomic with sync/atomic #40132

feat: replace custom atomic with sync/atomic #40132

Uh oh!

Conversation

kruskall commented Jul 8, 2024

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 8, 2024

Uh oh!

kruskall commented Jul 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efd6 Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efd6 Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmacknz commented Jul 17, 2024

Uh oh!

andrewkroh commented Jul 17, 2024

Uh oh!

leehinman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrzej-stencel commented Aug 13, 2024

Uh oh!

ycombinator commented Sep 3, 2024

Uh oh!

mergify bot commented Sep 21, 2024

Uh oh!

kruskall commented Nov 21, 2024

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

kruskall commented Dec 9, 2024

Uh oh!

vigneshshanmugam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants