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

Update go-client - new telemetry #1330

Merged
merged 6 commits into from
May 22, 2023
Merged

Update go-client - new telemetry #1330

merged 6 commits into from
May 22, 2023

Conversation

michaljurecko
Copy link
Contributor

@michaljurecko michaljurecko commented May 22, 2023 •
edited
Loading

Part of: https://keboola.atlassian.net/browse/PSGO-179

Changes:

  • Updated go-client.
  • Fixed code compatibility.

@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-179-update-go-client branch from 4b5a704 to 1e58e20 Compare May 22, 2023 07:27
@michaljurecko michaljurecko changed the title Michaljurecko psgo 179 update go client Update go-client - new telemetry May 22, 2023
@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-179-update-go-client branch 4 times, most recently from 249506e to e580f87 Compare May 22, 2023 08:23
@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-179-update-go-client branch 2 times, most recently from dbf5dad to b7bf083 Compare May 22, 2023 09:21
@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-179-update-go-client branch from b7bf083 to cbac529 Compare May 22, 2023 09:49
michaljurecko
michaljurecko commented May 22, 2023
View reviewed changes
Copy link
Contributor Author

@michaljurecko michaljurecko left a comment

Choose a reason for hiding this comment

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

V tomto PR aktualizujem go-client na verziu s telemetriou, a robim nevyhnutne upravy v kode.

go.mod Outdated
github.com/keboola/go-client v1.14.0
github.com/keboola/go-client v1.15.1-0.20230522083614-4f425c7ab9ef
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO, upravit verziu podla release, ked sa mergne:
keboola/go-client#95

internal/pkg/service/buffer/worker/service/slice_retry_test.go
Comment on lines -64 to 71
S3UploadParams: &s3.UploadParams{Path: s3.Path{Key: "foo", Bucket: "bar"}},
S3UploadParams: &s3.UploadParams{
Path: s3.Path{Key: "foo", Bucket: "bar"},
Credentials: s3.Credentials{
AccessKeyID: "foo",
SecretAccessKey: "bar",
},
},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Po aktualizacii zavislosti prestal fungovat tento test.
Testuje sa, ze upload na S3 spadne ... no ked nezadam ziadne credentials, tak teraz uz nespadne ako chcem, ale na nejakej validacii, ktora je skor.

internal/pkg/mapper/scheduler/remote.go
grp := client.NewRunGroup(ctx, keboolaProjectAPI.Client())
grp := request.NewRunGroup(ctx, keboolaProjectAPI.Client())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

V go-client sa rozdelil client pkg na: client a request, v tomto commite je upraveny kod aby reflektoval tuto zmenu.

internal/pkg/telemetry/metric/prometheus/prometheus.go
Comment on lines +60 to +62
// Register legacy OpenCensus metrics, for go-cloud (https://github.com/google/go-cloud/issues/2877)
exporter.RegisterProducer(opencensus.NewMetricProducer())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go-cloud, ktory pouzivame na pracu s S3/ABS nepodporuje zatial OpenTelemetry, viac v google/go-cloud#2877 (postupne je tych dovodou menej, casom urcite prejdu na OpenTelemetry). Zatial je tu tento bridge.

OpenCensus je zial globalna zalezitost, preto to registrujem na najvyssej moznej urovni, aby to nerozbilo testovatelnost kodu na nizsich urovniach.

internal/pkg/service/buffer/dependencies/dependencies.go
Comment on lines 48 to 51
httpClient := httpclient.New(
httpclient.WithTelemetry(tel),
httpclient.WithUserAgent(userAgent),
httpclient.WithEnvs(envs),
func(c *httpclient.Config) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

httpclient je pkg v kac repe, factory pre go-client klienta.

Pridavam tu podporu telemetri, ... envs sa nikde uz nepouzivali, tak som ich vyhodil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Niekedy sme mali envs na roznych miestach v kode, ... teraz ma kazda service uz svoj config, ale tu som envs zabudol zmazat.

internal/pkg/service/common/httpclient/httpclient.go
Comment on lines +63 to +77
// Enable telemetry
if conf.telemetry != nil {
cl = cl.WithTelemetry(
conf.telemetry.TracerProvider(),
conf.telemetry.MeterProvider(),
otel.WithRedactedHeaders("X-StorageAPI-Token", "X-KBC-ManageApiToken"),
otel.WithPropagators(
// DataDog supports multiple propagations: tracecontext, B3, legacy DataDog, ...
// W3C tracecontext propagator (propagation.TraceContext{}) is not working with the Storage API dd-trace-php ,
// so the B3 propagator is used.
b3.New(b3.WithInjectEncoding(b3.B3MultipleHeader)),
),
)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Namiesto DD telemetrie (low/high) sa pouzije telemetria, ktora je uz v go-client.
Management Token sa nikde nepouziva, nevolame management API, ale dal som ho sem preventivne.

internal/pkg/telemetry/oteldd/httpclient.go
Comment on lines -17 to -32
// DDTraceFactory provides TraceFactory for high-level tracing of the API client requests.
func DDTraceFactory() client.TraceFactory {
return func() *client.Trace {
t := &client.Trace{}

// Api request
var ctx context.Context
var clientRequest client.HTTPRequest // high-level request
var resultType string

var requestSpan ddtracer.Span
var parsingSpan ddtracer.Span
var retryDelaySpan ddtracer.Span

t.GotRequest = func(c context.Context, r client.HTTPRequest) context.Context {
clientRequest = r
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stara implementacie DD telemetrie pre httpclient moze ist prec.

@michaljurecko michaljurecko marked this pull request as ready for review May 22, 2023 10:04
@michaljurecko michaljurecko requested a review from zajca May 22, 2023 10:12
@michaljurecko michaljurecko merged commit 9446218 into main May 22, 2023
@michaljurecko michaljurecko deleted the michaljurecko-PSGO-179-update-go-client branch May 22, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zajca zajca zajca 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.
2 participants
@michaljurecko @zajca