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

tracing: fix DHT keys as string attribute not being valid utf-8 #859

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

MichaelMure
Copy link
Contributor

Fix #858

internal/tracing.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

This is just debug info but I'm not sure this is what we want here.
I would either always encode it or never do it, not smartly depending on if it's valid utf8 or not.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 27, 2023

Ok nvm, having the conditional is fine, let's still use a multibase to encode it.

Comment on lines +21 to +32
func KeyAsAttribute(name string, key string) attribute.KeyValue {
b := []byte(key)
if utf8.Valid(b) {
return attribute.String(name, key)
}
encoded, err := multibase.Encode(multibase.Base58BTC, b)
if err != nil {
// should be unreachable
panic(err)
}
return attribute.String(name, encoded)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like that we always run this code, even if we are not tracing. Might be something to do by returning an fmt.Stringer that lazily do this, however I belive otel always execute stringers anyway. The DHT is not very CPU efficient nor hot anyway so that fine, if it becomes not in profiles we should hoist this in some reusable package, send PR to lazily stringify in otel and return an fmt.Stringer.

@Jorropo Jorropo merged commit fa4953c into libp2p:master Jul 28, 2023
9 of 10 checks passed
@github-actions
Copy link

Suggested version: v0.24.3

Comparing to: v0.24.2 (diff)

Changes in go.mod file(s):

(empty)

gorelease says:

# diagnostics
go.mod: the following requirements are needed
	github.com/ipfs/[email protected]
	github.com/ipfs/[email protected]
Run 'go mod tidy' to add missing requirements.
required module github.com/microcosm-cc/[email protected] retracted by module author: Retract older versions as only latest is to be depended upon

# summary
Suggested version: v0.24.3

gocompat says:

Your branch is up to date with 'origin/master'.

Automatically created GitHub Release

Pre-creating GitHub Releases on release PRs initiated from forks is not supported.
If you wish to prepare release notes yourself, you should create a draft GitHub Release for tag v0.24.3 manually.
The draft GitHub Release is going to be published when this PR is merged.
If you choose not to create a draft GitHub Release, a published GitHub Released is going to be created when this PR is merged.

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.

runLookupWithFollowup has a trace with invalid utf-8 attribute
2 participants