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

feat: support opensearch as a tracing backend #1087

Merged
merged 11 commits into from
Aug 19, 2022
Merged

Conversation

mathnogueira
Copy link
Contributor

@mathnogueira mathnogueira commented Aug 17, 2022

This PR adds support for opensearch as a tracing backend.

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@@ -16,6 +16,50 @@ type Trace struct {
Flat map[trace.SpanID]*Span `json:"-"`
}

func NewTrace(traceID string, spans ...Span) Trace {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a card to our backlog so we use this method instead of building the trace manually when converting from the otel format.

@mathnogueira
Copy link
Contributor Author

I deleted the existing tracing backend tests because:

  1. They were being skipped anyway
  2. They didn't test anything, I rather think about another strategy for testing those (maybe running all tests using different data storages?)

@@ -115,8 +114,7 @@ func (pe DefaultPollerExecutor) donePollingTraces(job *PollingRequest, trace tra
return false
}

// We always expect to get the tracetest trigger span, so it has to have more than 1 span
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only true if the user configures tracetest to send the triggering span to the same data store as its application. So, to prevent infinite loops, I reverted the len() > 1 condition-

@mathnogueira mathnogueira linked an issue Aug 18, 2022 that may be closed by this pull request
Copy link
Contributor

@schoren schoren left a comment

Choose a reason for hiding this comment

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

LGTM. Left a few nit comments

@@ -168,4 +184,59 @@ services:
condition: service_healthy
jaeger:
condition: service_healthy
networks:
- local

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed? it's usually easier to let defaults do their thing

@@ -16,6 +16,50 @@ type Trace struct {
Flat map[trace.SpanID]*Span `json:"-"`
}

func NewTrace(traceID string, spans ...Span) Trace {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be named New, so the calls look like traces.New(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it looks easier to pass a []Span than to expand an array on every call. I wouldn't expect it to be called with a manual list of Spans

span.Attributes["tracetest.span.duration"] = getSpanDuration(*span)
}

func getSpanDuration(span Span) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename this to spanDuration so it's consistent with spanType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a spanDuration function right now here. I can rename it when I refactor the way we convert from OTEL to a trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the older function. We will delete it anyway in a near future

@mathnogueira mathnogueira merged commit 14289c8 into main Aug 19, 2022
@mathnogueira mathnogueira deleted the feat/open-search branch August 19, 2022 17:52
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.

Support OpenSearch as a backend trace datastore
2 participants