Skip to content
This repository was archived by the owner on Aug 13, 2025. It is now read-only.

Telemetry fine-tunning#95

Merged
michaljurecko merged 15 commits intomainfrom
michaljurecko-PSGO-204-otel-fine-tune-v2
May 22, 2023
Merged

Telemetry fine-tunning#95
michaljurecko merged 15 commits intomainfrom
michaljurecko-PSGO-204-otel-fine-tune-v2

Conversation

@michaljurecko
Copy link
Copy Markdown
Contributor

@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-204-otel-fine-tune-v2 branch from 303c0a0 to 15665de Compare May 22, 2023 05:32
@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-204-otel-fine-tune-v2 branch from 15665de to f339227 Compare May 22, 2023 06:15
@michaljurecko michaljurecko force-pushed the michaljurecko-PSGO-204-otel-fine-tune-v2 branch from f339227 to 127e117 Compare May 22, 2023 06:27
Copy link
Copy Markdown
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 su upravy, ktore som robil po nasadeni telemetrie na testing stack.

Span vyzera nejak takto:
image

Metriky sa pouzivaju v dashboarde, v casti ohladom HTTP klienta:

image

Comment thread pkg/client/tranport_test.go
Comment thread pkg/client/transport.go
Comment thread pkg/client/client.go
Comment on lines +124 to +128
// Tracer returns registered tracer, if any.
func (c Client) Tracer() otelTrace.Tracer {
return c.tracer
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

V tomto commite sa pridava tracing pre API request, ktory osahuje 1+ HTTP request.

V API request potrebujeme nejak ziskat tracer, spravil som to takto.

  • api request ->httpRequests[0] -> sender/client.Tracer()
  • API request ma [] HTTP request, zoberie sa prvy HTTP request.
  • HTTP request ma metodu Tracer(), z ktorej sa ziska tracer.
  • HTTP request ma zase referenciu na Sender (Client), ktory moze mat volitelne metodu Tracer.

Urobil som to tak, aby definicie zostali jednoduche a bez zmeny:

func (a *API) GetBranchRequest(key BranchKey) request.APIRequest[*Branch] {
result := &Branch{}
req := a.
newRequest(StorageAPI).
WithResult(result).
WithGet("dev-branches/{branchId}").
AndPathParam("branchId", key.ID.String())
return request.NewAPIRequest(result, req)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Je to taková zvláštní, že je to nalepené na ten první request, by mě přišlo přehlednější kdyby to byl nějaký batch a ten měl reqests a tracer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Batch tam je, to je ta high-level vrstva API request: request.NewAPIRequest(result, req1, req2, .... reqN)

Islo mi o to co najmenej komplikovat to definovanie requestov, to co navrhujes by na vsetkych miestach vyzeralo takto:
request.NewAPIRequest(result, a(API).TRACER, req1, req2, .... reqN). Ten tracer sa teraz berie z Client, ktory ma kazdy ten req1, req2 .... rovnaky.

Teoreticky by si mohol odosielat req1, req2 ... kazdy inym klientom, ale zmysel to velmi nedava.


Tj. vyhodnotil som to tak, ze mi dava vacsi zmysel nedavat tam jeden "zbytocny" argument, kedze to nema na nic vplyv, je to stale testovatelne, ale hej Je to taková zvláštní, (skusim este inu alternativu)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Upravene: #95 (comment)

Comment thread pkg/request/api_request.go
Comment thread pkg/request/api_request.go Outdated
Comment thread pkg/client/trace/otel/otel_test.go
Comment thread pkg/client/trace/otel/otel_test.go
Comment thread pkg/client/trace/otel/otel_test.go
Comment thread pkg/client/trace/otel/otel_test.go
Comment thread pkg/client/trace/otel/otel_test.go Outdated
Copy link
Copy Markdown
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 su upravy, ktore som robil po nasadeni telemetrie na testing stack.

Span vyzera nejak takto:
image

Metriky sa pouzivaju v dashboarde, v casti ohladom HTTP klienta:

image

@michaljurecko michaljurecko marked this pull request as ready for review May 22, 2023 06:47
@michaljurecko michaljurecko requested a review from zajca May 22, 2023 06:47
Copy link
Copy Markdown
Member

@zajca zajca left a comment

Choose a reason for hiding this comment

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

přijde mě dost zvláštní ten tracer na první request, ale jinak to vypadá v pohodě.

Comment on lines +138 to +150
// Get parent span, if any, otherwise a noopSpan is returned
parentSpan := trace.SpanFromContext(ctx)

// Get result type as string
var resultType string
if v := reflect.TypeOf(r.result); v != nil {
resultType = v.String()
}

// Create span
var span trace.Span
tracer := parentSpan.TracerProvider().Tracer("go-client-api-request")
ctx, span = tracer.Start(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zajca tak som to este upravil, Tracer sa nezoberie z prveho HTTP request, ale z ctx, z parentSpan, ak nejaky je, vid upravene testy.

@michaljurecko michaljurecko requested a review from zajca May 22, 2023 13:04
Copy link
Copy Markdown
Member

@zajca zajca left a comment

Choose a reason for hiding this comment

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

Jo takto mě to přijde o dost jasnější 👍

@michaljurecko michaljurecko merged commit 154d070 into main May 22, 2023
@michaljurecko michaljurecko deleted the michaljurecko-PSGO-204-otel-fine-tune-v2 branch May 22, 2023 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants