[pkg/ottl] Add MurmurHash3 converter #34155

kaisecheng · 2024-07-18T14:18:45Z

Description:

Add MurmurHash3 function to convert the target to a hexadecimal string of the murmurHash3 hash/digest
The hash result has cross checked with python and ruby

Link to tracking Issue:
#34077

Testing:

unit test
e2e test

Documentation:

readme
chlog

string of the murmurHash3 hash/digest Fix: open-telemetry#34077

linux-foundation-easycla · 2024-07-18T14:18:50Z

The committers listed above are authorized under a signed CLA.

✅ login: kaisecheng (8ecf228, f606b35, 26ee23c, 2aa64bf, 3b959b3, 31e7198, c53348c, 15d7046, 599858e, ebbaa00, a09240c, 63c08ea, 7e72396, c6a5bef)
✅ login: crobert-1 / name: Curtis Robert (fe7c1a8)

crobert-1 · 2024-07-18T15:49:21Z

Hello @kaisecheng, thanks for your PR! Just to let you know, I'm going to wait to review until the code owners of OTTL get a chance to triage the original issue to make sure it makes sense to add this functionality.

…emetry-collector-contrib into ottl_murmurhash3_func

pkg/ottl/ottlfuncs/README.md

crobert-1 · 2024-07-19T16:45:57Z

pkg/ottl/go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/json-iterator/go v1.1.12
 	github.com/open-telemetry/opentelemetry-collector-contrib/internal/coreinternal v0.105.0
 	github.com/open-telemetry/opentelemetry-collector-contrib/pkg/pdatatest v0.105.0
+	github.com/spaolacci/murmur3 v1.1.0


I'm a bit concerned in general about using spaolacci/murmur3. It has some open issues and PRs, but it appears to be dead at this point. Do you have thoughts of potentially using another package?

twmb/murmur3 could be an alternative. Initially, I was concerned about the license since it is not listed as BSD-3-Clause. However, after a closer look, it seems acceptable. I did a quick test, and it appears that the hash result from Sum128() requires the same byte flip as the current solution.
I don't see a significant benefit in switching the library, but I am open to change it.

One concern I have with `spaolacci/murmur3 - there is an outstanding issue, which I believe is the likely cause of some flaky test failures on previous CI runs in this PR.

AFAICT, the twmb/murmur3 is a fork initially created to specifically fix this issue

@robbavey you are right. will change to twmb/murmur3

crobert-1 · 2024-07-19T16:52:05Z

pkg/ottl/ottlfuncs/func_murmurhash3.go

+
+	version := v128
+	if !args.Version.IsEmpty() {
+		if (args.Version.Get() != v32) && (args.Version.Get() != v128) {


Nit: It might make sense to make this a switch statement with the error in the default case. This would allow the exhaustive linter to ensure we don't miss adding a new valid value in case of future changes.

crobert-1 · 2024-07-19T16:52:31Z

pkg/ottl/ottlfuncs/func_murmurhash3.go

+	v32  = "32"
+	v128 = "128" // default


Could we make these enum values? (To go along with my other nit about a switch statement)

Using an enum means the syntax to call the 32-bit version will be MurmurHash3("something", version=0). I think this is more confusing than MurmurHash3("something", version="32").

crobert-1 · 2024-07-19T16:54:58Z

pkg/ottl/ottlfuncs/func_murmurhash3.go

+
+	version := v128
+	if !args.Version.IsEmpty() {
+		if (args.Version.Get() != v32) && (args.Version.Get() != v128) {


Nit: It might make sense to just put args.Version.Get() into a new variable here, and check that value, and set version to it if it's valid, rather than calling the method so many times for the same value.

crobert-1 · 2024-07-19T17:06:02Z

pkg/ottl/ottlfuncs/func_murmurhash3.go

+
+// MurmurHash3HexString returns the hexadecimal representation of the hash in little-endian format.
+// MurmurHash3, developed by Austin Appleby, is sensitive to endianness. Unlike some other languages like Python,
+// which use little-endian for all architectures, the Go library `spaolacci/murmur3` has some open issues


It might be helpful to directly reference the open issues being referenced. That way in the future we could remove this if they're ever fixed, and just to be able to quickly see the underlying issue.

Co-authored-by: Curtis Robert <crobert@splunk.com>

TylerHelmuth

I'd like to continue discussing, via the issue, whether or not we need this function.

github-actions · 2024-08-14T05:20:24Z

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

karenzone · 2024-08-23T15:48:38Z

pkg/ottl/ottlfuncs/README.md

+If `version` is
+
+- `v32_hash`: Uses 32-bit version and returns a signed integer hash.
+- `v128_hash`: Use 128-bit version and returns an array of two signed integer hash.


Suggested change

- `v128_hash`: Use 128-bit version and returns an array of two signed integer hash.

- `v128_hash`: Uses 128-bit version and returns an array of two signed integer hash.

github-actions · 2024-09-08T05:20:55Z

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

github-actions · 2024-10-03T05:21:10Z

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

github-actions · 2024-10-18T05:20:57Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Add MurmurHash3 function to convert the target to a hexadecimal

3b959b3

string of the murmurHash3 hash/digest Fix: open-telemetry#34077

kaisecheng requested review from TylerHelmuth, bogdandrutu and evan-bradley as code owners July 18, 2024 14:18

kaisecheng requested a review from a team July 18, 2024 14:18

github-actions bot assigned crobert-1 Jul 18, 2024

github-actions bot added the pkg/ottl label Jul 18, 2024

github-actions bot requested a review from kentquirk July 18, 2024 14:19

update comment

15d7046

crobert-1 and others added 3 commits July 18, 2024 08:49

Merge branch 'main' into ottl_murmurhash3_func

fe7c1a8

add murmur3 to modules

f606b35

Merge branch 'ottl_murmurhash3_func' of github.com:kaisecheng/opentel…

8ecf228

…emetry-collector-contrib into ottl_murmurhash3_func

kaisecheng requested review from dmitryax, jpkrohling, mx-psi, songy23, crobert-1, mwear and djaglowski as code owners July 18, 2024 17:49

github-actions bot added cmd/oteltestbedcol connector/count connector/datadog connector/routing connector/sum exporter/datadog Datadog components exporter/elasticsearch exporter/honeycombmarker internal/filter labels Jul 18, 2024

github-actions bot requested review from greatestusername, JaredTan95, liustanley, mackjmr, shalper2 and ycombinator July 18, 2024 17:50

gotidy

599858e

crobert-1 reviewed Jul 19, 2024

View reviewed changes

kaisecheng and others added 6 commits July 19, 2024 18:46

lint

2aa64bf

Update pkg/ottl/ottlfuncs/README.md

26ee23c

Co-authored-by: Curtis Robert <crobert@splunk.com>

Update pkg/ottl/ottlfuncs/README.md

c53348c

Co-authored-by: Curtis Robert <crobert@splunk.com>

switch lib to twmb/murmur3

c6a5bef

switch lib to twmb/murmur3

a09240c

use switch case to check version

31e7198

TylerHelmuth requested changes Jul 19, 2024

View reviewed changes

kaisecheng added 3 commits July 22, 2024 15:29

tidy otelcontribcol

63c08ea

rename & comment

ebbaa00

add support to return hash in integer format

7e72396

github-actions bot added the cmd/otelcontribcol otelcontribcol command label Jul 30, 2024

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

karenzone reviewed Aug 23, 2024

View reviewed changes

github-actions bot removed the Stale label Aug 24, 2024

github-actions bot added the Stale label Sep 8, 2024

github-actions bot removed the Stale label Sep 19, 2024

github-actions bot added the Stale label Oct 3, 2024

github-actions bot closed this Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pkg/ottl] Add MurmurHash3 converter #34155

[pkg/ottl] Add MurmurHash3 converter #34155

kaisecheng commented Jul 18, 2024

linux-foundation-easycla bot commented Jul 18, 2024 •

edited

Loading

crobert-1 commented Jul 18, 2024

crobert-1 Jul 19, 2024

kaisecheng Jul 19, 2024

robbavey Jul 19, 2024

kaisecheng Jul 19, 2024

crobert-1 Jul 19, 2024

crobert-1 Jul 19, 2024

kaisecheng Jul 19, 2024

crobert-1 Jul 19, 2024

crobert-1 Jul 19, 2024

TylerHelmuth left a comment •

edited

Loading

github-actions bot commented Aug 14, 2024

karenzone Aug 23, 2024

github-actions bot commented Sep 8, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 18, 2024

	- `v128_hash`: Use 128-bit version and returns an array of two signed integer hash.
	- `v128_hash`: Uses 128-bit version and returns an array of two signed integer hash.

[pkg/ottl] Add MurmurHash3 converter #34155

[pkg/ottl] Add MurmurHash3 converter #34155

Conversation

kaisecheng commented Jul 18, 2024

linux-foundation-easycla bot commented Jul 18, 2024 • edited Loading

crobert-1 commented Jul 18, 2024

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

Choose a reason for hiding this comment

TylerHelmuth left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 18, 2024

linux-foundation-easycla bot commented Jul 18, 2024 •

edited

Loading

TylerHelmuth left a comment •

edited

Loading