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.

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

Add Otel Trace client interactions #6487

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

trajano
Copy link

@trajano trajano commented Jul 30, 2024

image

Should be okay now for the most part it works well see trajano/google-fonts-proxy-docker#7 (comment) for a screenshot

But there's one context.TODO() line I am not sure which context to pass in at that point.

@francislavoie francislavoie marked this pull request as draft July 30, 2024 20:50
@trajano
Copy link
Author

trajano commented Jul 31, 2024

I kinda want http.getconn to put the net.hostname as the span title so you don't have to drill down to see the value, but I can't seem to get that working yet.

@trajano trajano marked this pull request as ready for review August 7, 2024 18:55
@mholt
Copy link
Member

mholt commented Aug 23, 2024

Hey, sorry for the lack of response. I'm not really savvy with OTel, so I want to leave this for someone else to review.

@francislavoie Do you know who might be the best reviewer for this?

@trajano
Copy link
Author

trajano commented Aug 24, 2024

Odd the lint errors I am getting appear to be from places I didn't touch.

@mohammed90
Copy link
Member

Odd the lint errors I am getting appear to be from places I didn't touch.

They appeared with the new Go version. I'm fixing them locally, then you can merge once pushed and merged from my side.

@trajano
Copy link
Author

trajano commented Aug 25, 2024

@mohammed90 do you have a branch pushed I just merge from?

@mohammed90
Copy link
Member

@mohammed90 do you have a branch pushed I just merge from?

Patience is good

@mohammed90
Copy link
Member

@mohammed90 do you have a branch pushed I just merge from?

Patience is good

Confirmed to be false-positive in gosec bound-check: securego/gosec#1189.

Ignore the lint failure for now.

@trajano
Copy link
Author

trajano commented Aug 26, 2024

So we need a commit to adjust the rules first so my PR will be able to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants