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

refactor(gateway): consistent tracing names #261

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 5, 2023

In the context of ipfs-inactive/bifrost-gateway#68, I decided to make the tracing names more consistent. When we moved the gateway to boxo, and then on #176, we added and changed some of the tracings, but they were not consistent.

What I did here:

  • Prepended Handler. to all handler tracings, similar to what we had on the IPFSBackend with IPFSBackend..
  • Changed the tracer name to boxo/gateway to clearly indicate the sub-package and stopped pre-pending Gateway. in the tracing name, since we already have the gateway part in the tracer name..

@hacdias hacdias force-pushed the gateway/consistent-metrics branch from 7b67660 to c76cb01 Compare April 5, 2023 09:12
@hacdias hacdias changed the title refactor: make tracing namings consistent refactor(gateway): consistent tracing names Apr 5, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #261 (1a5f5ce) into main (5d6c73c) will decrease coverage by 0.04%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   47.89%   47.85%   -0.04%     
==========================================
  Files         273      273              
  Lines       33186    33186              
==========================================
- Hits        15893    15880      -13     
- Misses      15616    15624       +8     
- Partials     1677     1682       +5     
Impacted Files Coverage Δ
gateway/handler_block.go 0.00% <0.00%> (ø)
gateway/handler_car.go 0.00% <0.00%> (ø)
gateway/handler_codec.go 0.00% <0.00%> (ø)
gateway/handler_ipns_record.go 0.00% <0.00%> (ø)
gateway/handler_tar.go 0.00% <0.00%> (ø)
gateway/handler_defaults.go 36.66% <100.00%> (ø)
gateway/handler_unixfs_dir.go 62.89% <100.00%> (ø)
gateway/handler_unixfs_file.go 65.75% <100.00%> (ø)
gateway/metrics.go 57.76% <100.00%> (ø)

... and 7 files with indirect coverage changes

@hacdias hacdias marked this pull request as ready for review April 5, 2023 09:13
@hacdias hacdias requested a review from lidel as a code owner April 5, 2023 09:13
@hacdias hacdias self-assigned this Apr 5, 2023
@hacdias hacdias requested a review from aschmahmann April 5, 2023 09:13
@hacdias hacdias requested a review from Jorropo April 5, 2023 10:37
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@hacdias do you have a screenshot how it looks like in Jaeger UI when traced in Kubo or bifrost-gateway (whichever is easier for you)?

See https://github.com/ipfs/kubo/blob/master/docs/environment-variables.md#how-to-use-jaeger-ui
(examples in #233, just want to eyeball it so we are happy with how it looks)

@hacdias hacdias force-pushed the gateway/consistent-metrics branch from c76cb01 to 8e43931 Compare April 6, 2023 08:56
@hacdias
Copy link
Member Author

hacdias commented Apr 6, 2023

image

@lidel here it is. I re-added the Gateway. prefix. I thought the tracer name would be more prominent but it wasn't! It's better now! (also see ipfs/kubo#9797).

@hacdias hacdias requested a review from lidel April 6, 2023 08:57
@lidel lidel force-pushed the gateway/consistent-metrics branch from 85d963b to 1a5f5ce Compare April 12, 2023 00:39
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm

Screenshot 2023-04-12 at 02-49-44 Jaeger UI

@lidel lidel merged commit a12deee into main Apr 12, 2023
@lidel lidel deleted the gateway/consistent-metrics branch April 12, 2023 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants