Do not exceed ES _id length limit by tmszdmsk · Pull Request #905 · jaegertracing/jaeger

tmszdmsk · 2018-07-02T18:13:49Z

Which problem is this PR solving?

Short description of the changes

let's store a hash of the serviceId so that we don't exceed length limit of _id

Signed-off-by: Tomasz Adamski <tomasz.adamski@gmail.com>

codecov · 2018-07-02T18:31:49Z

Codecov Report

Merging #905 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #905   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         126    126           
  Lines        6070   6074    +4     
=====================================
+ Hits         6070   6074    +4

Impacted Files	Coverage Δ
plugin/storage/es/spanstore/service_operation.go	`100% <100%> (ø)`	⬆️
plugin/storage/es/spanstore/writer.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48bbbae...8e909f5. Read the comment docs.

Instead of using arbitrary string (which length can be quite big) as document _id we compute sha256 from it instead. Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

yurishkuro · 2018-07-04T16:25:35Z

plugin/storage/es/spanstore/service_operation.go


 import (
 	"context"
+	"crypto/sha256"


there's no need for crypto hash, see

jaeger/model/hash.go

Lines 30 to 36 in d16de31

func HashCode(o Hashable) (uint64, error) {

h := fnv.New64a()

if err := o.Hash(h); err != nil {

return 0, err

}

return h.Sum64(), nil

}

yurishkuro · 2018-07-04T16:29:48Z

plugin/storage/es/spanstore/service_operation.go

 	}
 	serviceID := fmt.Sprintf("%s|%s", service.ServiceName, service.OperationName)
+	hashedID := fmt.Sprintf("%x", sha256.Sum256([]byte(serviceID)))
 	cacheKey := fmt.Sprintf("%s:%s", indexName, serviceID)


I don't see why we cannot use the hash for both document ID and cacheKey. Let's just have a single value.

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

harnash · 2018-07-05T08:51:43Z

@yurishkuro thanks for the comments. I hopefully addressed them all in 10fcf67

yurishkuro · 2018-07-05T15:36:34Z

plugin/storage/es/spanstore/service_operation_test.go

+			OperationName: "operation",
+		}
+
+		serviceHash, err := model.HashCode(service)


this introduces unnecessary public method to Service type. Instead I would simply add something like this:

func (s Service) hashCode() string { h := fnv.New64a() h.Write(s.ServiceName) h.Write(s. OperationName) return fmt.Sprintf("%x", h.Sum64()) }

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

black-adder · 2018-07-09T15:22:33Z

plugin/storage/es/spanstore/writer.go

 }

+func (s Service) hashCode() string {
+	h := fnv.New64a()


can this be cached?

@black-adder It could be but it will introduce issue with concurrency. Every simultaneous call to this function would potentially cause race condition and would render unpredictable hash results. I would need to introduce some kind of semaphore and mark this part as critical section. Do you think it is worth?

unpredictable hash results -> shouldn't this always produce the same hash results for the same key even if run concurrently? I actually dont know the inner workings of the implementation so maybe completely wrong, we can skip for now but can you add a TODO to investigate in the future (unless you already know that there will be race conditions).

@black-adder so my reasoning is like this.

You store cached hasher object somewhere (most likely in the writer.go) and use it whenever someone calls hashCode()

First process (1) calls hashCode(), fetches (creates if not cached) hasher object

(1) Pushes data: OperationName and ServiceName to the hasher

Second process (2) calls hashCode() which fetches the same (cached) hasher object

(2) pushes its OperationName and ServiceName to the same hasher

(1) calculates checksum by calling Sum64() on the same hasher (which now holds data from two possibly different instances of Service)

(2) does the calculations too and gets the same (wrong) hash as (1)

I did actually run into this issue while fixing the tests :-)

Even if I introduce hash.Reset() just after Sum64() or before data is pushed it the hasher I still have no guarantees that other process is not doing some operation on the cached object. Putting Write semaphore would probably do the trick but I think it is a bit overkill in this case.

I hope this helps.

black-adder · 2018-07-09T15:24:59Z

plugin/storage/es/spanstore/service_operation_test.go

 		indexService.On("Index", stringMatcher(indexName)).Return(indexService)
 		indexService.On("Type", stringMatcher(serviceType)).Return(indexService)
-		indexService.On("Id", stringMatcher("service|operation")).Return(indexService)
+		indexService.On("Id", stringMatcher(serviceHash)).Return(indexService)


I'm not a big fan of using the same hash function here to test that the service was hashed, I'd prefer if you generated this hash once and compare against the actual hash string.

I've removed calls to hashCode() and put static hash in those tests.

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

leave document _id generation to Eleasticsearch

6f43001

Signed-off-by: Tomasz Adamski <tomasz.adamski@gmail.com>

tmszdmsk requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners July 2, 2018 18:13

tmszdmsk force-pushed the 779-ES-autogenerate-id branch from e0dfe2a to 6f43001 Compare July 2, 2018 18:14

tmszdmsk changed the title ~~leave document _id generation to Eleasticsearch~~ don't exceed ES _id length limit Jul 4, 2018

harnash force-pushed the 779-ES-autogenerate-id branch 2 times, most recently from 44ffbdf to af89809 Compare July 4, 2018 08:51

Calculate sha256 checksum for document _id for Elasticsearch.

3489723

Instead of using arbitrary string (which length can be quite big) as document _id we compute sha256 from it instead. Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

harnash force-pushed the 779-ES-autogenerate-id branch from af89809 to 3489723 Compare July 4, 2018 09:23

yurishkuro reviewed Jul 4, 2018

View reviewed changes

Using Hashable interface to calculate Elasticsearch ids

10fcf67

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

harnash force-pushed the 779-ES-autogenerate-id branch from c007a8e to 10fcf67 Compare July 5, 2018 08:50

yurishkuro reviewed Jul 5, 2018

View reviewed changes

harnash and others added 2 commits July 9, 2018 15:31

Introduce Service::hashCode() to calculate Elasticsearch _id

762c739

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

Merge branch 'master' into 779-ES-autogenerate-id

658cec6

black-adder reviewed Jul 9, 2018

View reviewed changes

harnash and others added 2 commits July 10, 2018 11:00

Do not calculate Service hash in tests

3d46c8a

Signed-off-by: Łukasz Harasimowicz <dev@harnash.eu>

Merge branch 'master' into 779-ES-autogenerate-id

8e909f5

yurishkuro approved these changes Jul 10, 2018

View reviewed changes

yurishkuro merged commit 9ffc2fc into jaegertracing:master Jul 10, 2018

yurishkuro changed the title ~~don't exceed ES _id length limit~~ Do not exceed ES _id length limit Jul 10, 2018

This was referenced Jul 10, 2018

Jaeger-collector stops storing traces #897

Closed

Collector (ES Storage) error when trying to store trace with long span names #779

Closed

tmszdmsk deleted the 779-ES-autogenerate-id branch July 10, 2018 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not exceed ES _id length limit#905

Do not exceed ES _id length limit#905
yurishkuro merged 7 commits intojaegertracing:masterfrom
tmszdmsk:779-ES-autogenerate-id

tmszdmsk commented Jul 2, 2018 •

edited

Loading

Uh oh!

codecov bot commented Jul 2, 2018 •

edited

Loading

Uh oh!

yurishkuro Jul 4, 2018

Uh oh!

yurishkuro Jul 4, 2018

Uh oh!

harnash commented Jul 5, 2018

Uh oh!

yurishkuro Jul 5, 2018

Uh oh!

black-adder Jul 9, 2018

Uh oh!

harnash Jul 10, 2018

Uh oh!

black-adder Jul 10, 2018

Uh oh!

harnash Jul 12, 2018

Uh oh!

black-adder Jul 9, 2018

Uh oh!

harnash Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	func HashCode(o Hashable) (uint64, error) {
	h := fnv.New64a()
	if err := o.Hash(h); err != nil {
	return 0, err
	}
	return h.Sum64(), nil
	}

Conversation

tmszdmsk commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

codecov bot commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harnash commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tmszdmsk commented Jul 2, 2018 •

edited

Loading

codecov bot commented Jul 2, 2018 •

edited

Loading