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.

Sign up for GitHub

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

Jump to bottom

add Tracer for OpenCensus #402

Merged
merged 9 commits into from
Nov 2, 2018
Merged

add Tracer for OpenCensus #402

merged 9 commits into from
Nov 2, 2018

Conversation

vvakame
Copy link
Collaborator

@vvakame vvakame commented Oct 29, 2018

add Tracer implementation for OpenCensus.
I choose package name that gqlopencensus. because do not duplicate names with other packages.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@vvakame vvakame requested a review from vektah October 29, 2018 08:17
@vvakame
Copy link
Collaborator Author

vvakame commented Oct 29, 2018

https://circleci.com/gh/99designs/gqlgen/786
wont_leak_goroutines is little bit flaky...? 🤔

--- FAIL: TestGeneratedServer (0.23s)
    --- FAIL: TestGeneratedServer/subscriptions (0.21s)
        --- FAIL: TestGeneratedServer/subscriptions/wont_leak_goroutines (0.20s)
	Error Trace:	generated_test.go:270
	Error:      	Not equal: 
	            	expected: 9
	            	actual  : 8
	Test:       	TestGeneratedServer/subscriptions/wont_leak_goroutines

@vektah
Copy link
Collaborator

vektah commented Oct 29, 2018

wont_leak_goroutines is little bit flaky...? 🤔

Yeah, probably need to bump the wait time, or poll with a longer timeout.

vektah
vektah reviewed Oct 30, 2018
View reviewed changes
gqlopencensus/option.go Outdated
}

// Option is anything that can configure Tracer.
type Option interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for consistency with the rest of gqlgen, this should be:

type Option func(cfg *config)

gqlopencensus/option.go Outdated
}

// WithStartOperationExecution returns option that execute some process on StartOperationExecution step.
func WithStartOperationExecution(f func(ctx context.Context) context.Context) Option {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what all are these for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that it would be better for developers to have hooks if they want to add something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already an interface there, this should be batteries included but opinionated, and if the user wants more they should be able to write their own in <100 lines.

More than half this PR is just hooks that aren't being used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make sense. I'll omit it later.

gqlopencensus/tracer.go

func (t *tracerImpl) StartFieldExecution(ctx context.Context, field graphql.CollectedField) context.Context {
ctx, span := trace.StartSpan(ctx, field.ObjectDefinition.Name+"/"+field.Name)
if !span.IsRecordingEvents() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does opencencus handle a request that wasn't sampled, but ends up causing an error? does try to promote it to being sampled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If our code doesn't have if !span.IsRecordingEvents() section, It works fine as well too.
OpenCensus doesn't change API behavior by sampling or not.
https://godoc.org/go.opencensus.io/trace#Span.IsRecordingEvents

@vvakame
Copy link
Collaborator Author

vvakame commented Oct 31, 2018

I just pushed some commit 👀

@vvakame
Copy link
Collaborator Author

vvakame commented Oct 31, 2018

NOTE: This change needs update when after #404 merged.

@vvakame
Copy link
Collaborator Author

vvakame commented Nov 2, 2018 •
edited
Loading

ah... I want to add some code after #403 merged.

@vvakame
Copy link
Collaborator Author

vvakame commented Nov 2, 2018

ready for merge!

@vektah vektah merged commit d264858 into master Nov 2, 2018
@vektah vektah deleted the feat-opencensus branch November 2, 2018 05:13
@vvakame
Copy link
Collaborator Author

vvakame commented Nov 2, 2018

@vektah thanks to fix CI!

cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
@vektah
Merge pull request 99designs#402 from 99designs/feat-opencensus
10ed803 
add Tracer for OpenCensus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vektah vektah vektah left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vvakame @vektah