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

contrib/graph-gophers/graphql-go: exposing operation name in the tracer #586

Merged
merged 5 commits into from
Feb 14, 2020
Merged

contrib/graph-gophers/graphql-go: exposing operation name in the tracer #586

merged 5 commits into from
Feb 14, 2020

Conversation

mikea
Copy link
Contributor

@mikea mikea commented Feb 6, 2020 •
edited
Loading

Fixes #590

@mikea
Copy link
Contributor Author

mikea commented Feb 6, 2020

Not sure why does the build fail:

#!/bin/bash -eo pipefail
go run checkmilestone.go

Milestone not set.
exit status 1

Exited with code exit status 1

gbbr
gbbr suggested changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Even though this change is probably ok to introduce, do you mind following our contribution guidelines and first opening an issue to discuss your proposal, before committing to the actual work?

@mikea mikea mentioned this pull request Feb 7, 2020
Closed
@mikea
Copy link
Contributor Author

mikea commented Feb 7, 2020

Even though this change is probably ok to introduce, do you mind following our contribution guidelines and first opening an issue to discuss your proposal, before committing to the actual work?

Sure: #590

@mikea mikea changed the title Graphql-Go: exposing operation name in the tracer contrib/graph-gophers/graphql-go: exposing operation name in the tracer Feb 7, 2020
@gbbr gbbr requested a review from knusbaum February 12, 2020 11:44
@gbbr
Copy link
Contributor

gbbr commented Feb 12, 2020

@knusbaum can you PTAL?

@mikea mikea requested a review from gbbr February 12, 2020 18:52
knusbaum
knusbaum requested changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Thanks. There is only one small tweak that I think should be made.

contrib/graph-gophers/graphql-go/graphql.go Outdated Show resolved Hide resolved
@knusbaum knusbaum added this to the 1.22.0 milestone Feb 12, 2020
@mikea mikea force-pushed the graphql_go_operation_name branch from e78a5d9 to 568a664 Compare February 12, 2020 21:40
@mikea mikea requested a review from knusbaum February 12, 2020 21:41
@knusbaum
Copy link
Contributor

I'm working on fixing CI here: #595

knusbaum
knusbaum approved these changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

LGTM pending CI passing.

@mikea
Copy link
Contributor Author

mikea commented Feb 12, 2020 •
edited
Loading

LGTM pending CI passing.

Head build is currently broken.

Build failure seems to come from kubernetes support:

# gopkg.in/DataDog/dd-trace-go.v1/contrib/k8s.io/client-go/kubernetes [gopkg.in/DataDog/dd-trace-go.v1/contrib/k8s.io/client-go/kubernetes.test]
contrib/k8s.io/client-go/kubernetes/kubernetes_test.go:65:35: not enough arguments in call to client.CoreV1().Namespaces().List
	have ("k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
	want (context.Context, "k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
contrib/k8s.io/client-go/kubernetes/kubernetes_test.go:99:36: not enough arguments in call to client.CoreV1().Namespaces().List
	have ("k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
	want (context.Context, "k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)

graphql-go tests are fine:

ok  	gopkg.in/DataDog/dd-trace-go.v1/contrib/graph-gophers/graphql-go	1.157s

https://circleci.com/gh/DataDog/dd-trace-go/3316?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@knusbaum

@knusbaum
Copy link
Contributor

Yes, there's been a change in k8s.io/client-go upstream. I have an open PR to fix it (#595)

gbbr
gbbr suggested changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I've updated your branch, hopefully CI will pass now. Also asking for a small change...

contrib/graph-gophers/graphql-go/graphql.go Show resolved Hide resolved
gbbr
gbbr approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

LGTM :)

@mikea
Copy link
Contributor Author

mikea commented Feb 13, 2020

@gbbr @knusbaum
Gabriel, Kyle, thanks for review.
Please merge this at your convenience.

@gbbr gbbr merged commit 2a7e054 into DataDog:v1 Feb 14, 2020
@gbbr gbbr added this to the 1.22.0 milestone Feb 17, 2020
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
@mikea @mingrammer
contrib/graph-gophers/graphql-go: add operation name as a tag (DataD…
022ee59 
…og#586)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knusbaum knusbaum knusbaum approved these changes

@gbbr gbbr gbbr approved these changes

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

Successfully merging this pull request may close these issues.

contrib/graph-gophers/graphql-go: tracing integration to expose operation name
3 participants
@mikea @gbbr @knusbaum