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

support Elastic APM for tracing #1482

Merged
merged 5 commits into from
Sep 4, 2019
Merged

support Elastic APM for tracing #1482

merged 5 commits into from
Sep 4, 2019

Conversation

Wing924
Copy link
Contributor

@Wing924 Wing924 commented Sep 2, 2019

Signed-off-by: Wing924 weihe924stephen@gmail.com

  • [] CHANGELOG entry if change is relevant to the end user.

Changes

  • support elastic APM for tracing

Verification

@Wing924
support Elastic APM for tracing
fd795e3 
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
@Wing924
fix docs
c4878dc 
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
@Wing924
retry job
75bc037 
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
@Wing924
Copy link
Contributor Author

Wing924 commented Sep 2, 2019

Test failed because

# cd .; git clone https://git.apache.org/thrift.git /tmp/thanos-go/src/git.apache.org/thrift.git
Cloning into '/tmp/thanos-go/src/git.apache.org/thrift.git'...
fatal: unable to access 'https://git.apache.org/thrift.git/': Failed to connect to git.apache.org port 443: Connection timed out
package github.com/minio/minio
	imports git.apache.org/thrift.git/lib/go/thrift: exit status 128

bwplotka
bwplotka approved these changes Sep 2, 2019
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM and thank your!

Super minot nit, otherwise perfect ❤️

I restarted CI, a flake indeed.

go.sum
github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af h1:pmfjZENx5imkbgOkpRUYLnmbU7UEFbjtDA2hxJ1ichM=
github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
github.com/joeshaw/multierror v0.0.0-20140124173710-69b34d4ec901 h1:rp+c0RAYOWj8l6qbCUTSiRLG/iKnW3K3/QfPPuSsBt4=
github.com/joeshaw/multierror v0.0.0-20140124173710-69b34d4ec901/go.mod h1:Z86h9688Y0wesXCyonoVr47MasHilkuLMqGhRZ4Hpak=
Copy link
Member

Choose a reason for hiding this comment

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

..another multi error package to the family ;p

Nothing we can do, but just funny there are so many libs for a few lines code logic (:

pkg/tracing/client/factory.go Outdated
@@ -5,6 +5,8 @@ import (
"io"
"strings"

"github.com/thanos-io/thanos/pkg/tracing/elasticapm"

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this newline and run make format? (limitation of goimports)

scripts/cfggen/main.go Outdated
@@ -8,6 +8,8 @@ import (
"reflect"
"strings"

"github.com/thanos-io/thanos/pkg/tracing/elasticapm"

Copy link
Member

Choose a reason for hiding this comment

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

ditto

@Wing924
format code
94f9204 
Signed-off-by: Wing924 <weihe924stephen@gmail.com>
@Wing924
Copy link
Contributor Author

Wing924 commented Sep 2, 2019

@bwplotka I fixed the code format. please review it again.

@bwplotka
Copy link
Member

bwplotka commented Sep 2, 2019

Awesome. CI is clearly blocked we are working on the fix for: #1483

@Wing924
Copy link
Contributor Author

Wing924 commented Sep 4, 2019

@bwplotka
CI is passed. Could you tell me when will you merge or decline it?

@bwplotka bwplotka merged commit 733e9fa into thanos-io:master Sep 4, 2019
@bwplotka
Copy link
Member

bwplotka commented Sep 4, 2019

Already approved so.. why decline? (:

Nice job, thanks!

@Wing924 Wing924 deleted the elastic_apm branch September 4, 2019 12:00
wbh1 pushed a commit to wbh1/thanos that referenced this pull request Sep 17, 2019
@Wing924 @wbh1
support Elastic APM for tracing (thanos-io#1482)
fa21edd 
* support Elastic APM for tracing

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

* fix docs

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

* retry job

Signed-off-by: Wing924 <weihe924stephen@gmail.com>

* format code

Signed-off-by: Wing924 <weihe924stephen@gmail.com>
@bwplotka bwplotka mentioned this pull request Oct 30, 2019
Closed
@szediktam
Copy link

@Wing924 hi, some questions about elastic_apm config.

I setup a apm server in a k8s cluster, and conntect to elasticsearch, also show "You have correctly setup APM Server" in kibana apm.
I also set a flag in thanos-query as following:
--tracing.config=type: ELASTIC_APM config: service_name: "query-trace" service_version: "1.0.1" service_environment: "container" sample_rate: 1

But there is no apm agent in kibana. something else to do?
thanos-query is a apm agent, or a apm agent need to setup?

thanks.

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

@bwplotka bwplotka bwplotka approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Wing924 @bwplotka @szediktam