Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sampling/eventstorage: introduce event storage #4108

Merged
merged 6 commits into from
Sep 1, 2020

Conversation

axw
Copy link
Member

@axw axw commented Aug 26, 2020

Motivation/summary

Introduce a package which provides local event storage, tailored for tail-based sampling. Storage is backed by Badger (github.com/dgraph-io/badger).

The Storage type provides two types for readiing/writing data: ReadWriter, and ShardedReadWriter. ReadWriter is not safe for concurrent access, while ShardedReadWriter is. ShardedReadWriter provides locked access to one of several ReadWriters, sharding on trace ID.

Currently we encode transactions and spans as JSON. We should look into developing a more efficient codec later, using protobuf or similar.

I've struck out metrics/monitoring for now, we should do that in a follow-up.

Checklist

I have considered changes for:
- [ ] documentation
- [ ] logging (add log lines, choose appropriate log selector, etc.)
- [ ] metrics and monitoring (create issue for Kibana team to add metrics to visualizations, e.g. Kibana#44001)

How to test these changes

make test

Related issues

None.

@apmmachine
Copy link
Contributor

apmmachine commented Aug 26, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #4108 updated]

  • Start Time: 2020-09-01T02:49:40.741+0000

  • Duration: 46 min 54 sec

Test stats 🧪

Test Results
Failed 0
Passed 3036
Skipped 139
Total 3175

Steps errors

Expand to view the steps failures

  • Name: Compress

    • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

    • Duration: 0 min 0 sec

    • Start Time: 2020-09-01T03:02:43.386+0000

    • log

  • Name: Compress

    • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

    • Duration: 0 min 0 sec

    • Start Time: 2020-09-01T03:17:59.115+0000

    • log

  • Name: Test Sync

    • Description: ./script/jenkins/sync.sh

    • Duration: 3 min 50 sec

    • Start Time: 2020-09-01T02:59:37.368+0000

    • log

Introduce a package which provides local event storage,
tailored for tail-based sampling. Storage is backed by
Badger (github.com/dgraph-io/badger).

The Storage type provides two types for readiing/writing
data: ReadWriter, and ShardedReadWriter. ReadWriter is
not safe for concurrent access, while ShardedReadWriter is.
ShardedReadWriter provides locked access to one of several
ReadWriters, sharding on trace ID.

Currently we encode transactions and spans as JSON.
We should look into developing a more efficient codec
later, using protobuf or similar.
@axw axw force-pushed the tailsampling-eventstorage branch from 5eafce8 to eeb1595 Compare August 26, 2020 10:09
@axw axw marked this pull request as ready for review August 26, 2020 11:47
@axw axw requested a review from a team August 26, 2020 11:47
x-pack/apm-server/sampling/eventstorage/storage.go Outdated Show resolved Hide resolved
x-pack/apm-server/sampling/eventstorage/storage.go Outdated Show resolved Hide resolved
}

// ReadEvents reads events with the given trace ID from storage into a batch.
func (rw *ReadWriter) ReadEvents(traceID string, out *model.Batch) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ReadEvents might flush pending writes and it is generally not safe for concurrent usage, a comment would be helpful here, indicating that this would require a write lock for safe concurrent usage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ReadWriter type's doc comment already states the whole type is not safe for concurrent usage. Is there still something unclear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not necessarily expect a write action on a ReadEvents method, so when using and locking it in the caller, there might only be a read lock used and not a write lock. But one would probably look at the code and realize that, so it's ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, understood - I'll add a comment to ReadEvents stating that it may also cause writes. I don't think we'll end up with read/write locks as the event storage is write-heavy, and ReadEvents is only called periodically.

x-pack/apm-server/sampling/eventstorage/storage.go Outdated Show resolved Hide resolved
axw added 2 commits August 31, 2020 16:39
T for Transaction
S for Span
u for unsampled
s for sampled
@axw axw merged commit 3f8abfa into elastic:master Sep 1, 2020
@axw axw deleted the tailsampling-eventstorage branch September 1, 2020 05:34
axw added a commit to axw/apm-server that referenced this pull request Sep 8, 2020
* sampling/eventstorage: introduce event storage

Introduce a package which provides local event storage,
tailored for tail-based sampling. Storage is backed by
Badger (github.com/dgraph-io/badger).

The Storage type provides two types for readiing/writing
data: ReadWriter, and ShardedReadWriter. ReadWriter is
not safe for concurrent access, while ShardedReadWriter is.
ShardedReadWriter provides locked access to one of several
ReadWriters, sharding on trace ID.

Currently we encode transactions and spans as JSON.
We should look into developing a more efficient codec
later, using protobuf or similar.
axw added a commit that referenced this pull request Sep 8, 2020
* sampling/eventstorage: introduce event storage

Introduce a package which provides local event storage,
tailored for tail-based sampling. Storage is backed by
Badger (github.com/dgraph-io/badger).

The Storage type provides two types for readiing/writing
data: ReadWriter, and ShardedReadWriter. ReadWriter is
not safe for concurrent access, while ShardedReadWriter is.
ShardedReadWriter provides locked access to one of several
ReadWriters, sharding on trace ID.

Currently we encode transactions and spans as JSON.
We should look into developing a more efficient codec
later, using protobuf or similar.
@axw axw mentioned this pull request Sep 10, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants