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

Support archive storage in the query-service #604

Merged
merged 2 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@yurishkuro
Member

yurishkuro commented Dec 20, 2017

Users often ask for this feature. The trace retention period is usually short, and often people would put a link to the trace into some issue tracker to improve service performance, but by the time someone starts working on the ticket the trace has already expired.

Resolves #603

yurishkuro added some commits Jan 8, 2018

Support archive storage in the query-service
Signed-off-by: Yuri Shkuro <ys@uber.com>
fmt
Signed-off-by: Yuri Shkuro <ys@uber.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling a2a80d1 on archive-storage into 8db7a11 on master.

coveralls commented Jan 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling a2a80d1 on archive-storage into 8db7a11 on master.

@yurishkuro yurishkuro changed the title from [WIP] Support archive storage in the query-service to Support archive storage in the query-service Jan 8, 2018

@yurishkuro

This comment has been minimized.

Show comment
Hide comment
@yurishkuro

yurishkuro Jan 8, 2018

Member

ready for review

Member

yurishkuro commented Jan 8, 2018

ready for review

func archiveOptions(storageFactory istorage.Factory, logger *zap.Logger) []app.HandlerOption {
reader, err := storageFactory.CreateSpanReader()
if err == istorage.ErrArchiveStorageNotConfigured || err == istorage.ErrArchiveStorageNotSupported {
return nil

This comment has been minimized.

@vprithvi

vprithvi Jan 8, 2018

Member

Wouldn't this fail silently when ErrArchiveStorageNotConfigured is false; meaning it's configured; and when ErrArchiveStorageNotSupported is true?
Shouldn't we return the ErrArchiveStorageNotSupported when someone mistakenly configures archive storage on a storage that doesn't support archiving?

@vprithvi

vprithvi Jan 8, 2018

Member

Wouldn't this fail silently when ErrArchiveStorageNotConfigured is false; meaning it's configured; and when ErrArchiveStorageNotSupported is true?
Shouldn't we return the ErrArchiveStorageNotSupported when someone mistakenly configures archive storage on a storage that doesn't support archiving?

This comment has been minimized.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it's not possible to configure archiving on a storage that doesn't support it. For example, with Cassandra you need to pass --cassandra-archive.enabled=true.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it's not possible to configure archiving on a storage that doesn't support it. For example, with Cassandra you need to pass --cassandra-archive.enabled=true.

if f.archiveSession == nil {
return nil, storage.ErrArchiveStorageNotConfigured
}
return cSpanStore.NewSpanWriter(f.archiveSession, f.Options.SpanStoreWriteCacheTTL, f.metricsFactory, f.logger), nil

This comment has been minimized.

@vprithvi

vprithvi Jan 8, 2018

Member

I'm a bit confused, is f.Options.SpanStoreWriteCacheTTL shared between both the archive and primary span writers?

@vprithvi

vprithvi Jan 8, 2018

Member

I'm a bit confused, is f.Options.SpanStoreWriteCacheTTL shared between both the archive and primary span writers?

This comment has been minimized.

@yurishkuro

yurishkuro Jan 8, 2018

Member

at the moment, yes. The retention period of archive storage is expected to be much longer that for primary, so the short cache TTL for the primary is fine for archive too. Strictly speaking we don't even need this cache for archive, but whatever.

@yurishkuro

yurishkuro Jan 8, 2018

Member

at the moment, yes. The retention period of archive storage is expected to be much longer that for primary, so the short cache TTL for the primary is fine for archive too. Strictly speaking we don't even need this cache for archive, but whatever.

@@ -38,13 +44,14 @@ type Factory struct {
primaryConfig config.SessionBuilder
primarySession cassandra.Session
// archiveSession cassandra.Session TODO
archiveConfig config.SessionBuilder

This comment has been minimized.

@vprithvi

vprithvi Jan 8, 2018

Member

I feel that the archiveConfig can be set to a higher default consistency level than the primaryConfig. What do you think?

@vprithvi

vprithvi Jan 8, 2018

Member

I feel that the archiveConfig can be set to a higher default consistency level than the primaryConfig. What do you think?

This comment has been minimized.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it's fully configurable by user. Given how marginal this feature is overall, I don't think introducing custom defaults per storage class is worth the complexity in the code.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it's fully configurable by user. Given how marginal this feature is overall, I don't think introducing custom defaults per storage class is worth the complexity in the code.

@black-adder

do you wanna document this anywhere?

@@ -25,10 +25,12 @@ import (
"github.com/jaegertracing/jaeger/pkg/cassandra"
"github.com/jaegertracing/jaeger/pkg/cassandra/mocks"
"github.com/jaegertracing/jaeger/pkg/config"

This comment has been minimized.

@black-adder

black-adder Jan 8, 2018

Collaborator

?

@black-adder

black-adder Jan 8, 2018

Collaborator

?

This comment has been minimized.

@yurishkuro

yurishkuro Jan 8, 2018

Member

ppppphhhhh

@yurishkuro

yurishkuro Jan 8, 2018

Member

ppppphhhhh

// CreateArchiveSpanReader implements storage.ArchiveFactory
func (f *Factory) CreateArchiveSpanReader() (spanstore.Reader, error) {
factory, ok := f.factories[f.SpanStorageType]

This comment has been minimized.

@black-adder

black-adder Jan 8, 2018

Collaborator

this means that the span store and archive span store both have to be using the same storage type? I think that's a fair assumption

@black-adder

black-adder Jan 8, 2018

Collaborator

this means that the span store and archive span store both have to be using the same storage type? I think that's a fair assumption

This comment has been minimized.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it can be changed in the future by introducing another env var.

@yurishkuro

yurishkuro Jan 8, 2018

Member

it can be changed in the future by introducing another env var.

@yurishkuro

This comment has been minimized.

Show comment
Hide comment
@yurishkuro

yurishkuro Jan 8, 2018

Member

do you wanna document this anywhere?

yes, once the UI support is added to actually archive traces (jaegertracing/jaeger-ui#7).

Member

yurishkuro commented Jan 8, 2018

do you wanna document this anywhere?

yes, once the UI support is added to actually archive traces (jaegertracing/jaeger-ui#7).

@yurishkuro yurishkuro merged commit 7f1b9a2 into master Jan 8, 2018

4 of 5 checks passed

License Compliance 12 issues found.
Details
DCO All commits have a DCO sign-off from the author
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@wafflebot wafflebot bot removed the review label Jan 8, 2018

@yurishkuro yurishkuro deleted the archive-storage branch May 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment