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: add highlevel APIs records on the composable routers #80

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Aug 19, 2023

No description provided.

@dennis-tra
Copy link
Contributor

dennis-tra commented Aug 25, 2023

Haven't thought this through but what about adding a "traced" router that wraps another routing.Routing but also implements the routing.Routing interface. It would just call through to the wrapped router but surrounded by traces instructions.

I'm bringing this up because I have done something similar in https://github.com/ipfs/go-datastore/pull/209/files just recently (not merged yet).

@Jorropo
Copy link
Contributor Author

Jorropo commented Aug 25, 2023

Haven't thought this through but what about adding a "traced" router that wraps another routing.Routing but also implements the routing.Routing interface. It would just call through to the wrapped router but surrounded by traces instructions.

I did thought about doing this, but it's more annoying to use.
If anything we could have both options.

@Jorropo
Copy link
Contributor Author

Jorropo commented Sep 4, 2023

@dennis-tra is that good enough, can I merge ?
This is unifying, moving and making better some logic that already exists in go-libp2p-kad-dht.
Someone else can write the wrapper code later if they care about it.

@dennis-tra
Copy link
Contributor

Two things:

  1. I'm not too familiar with the otel API but in my linked PR for go-datastore I also added RecordError "to record err as an exception span event for this span". Whatever this means. Feel free to judge if that's needed or not (source).
  2. I still think a tracedrouter or whatever that implements all the different interfaces and wraps another routing.Routing etc. would be the nicer pattern. However, I can see that it could become annoying to hook into the various async operations of the wrapped routing.Routing, so that's fine with me 👍 also, you're right, it's trivial to add if someone really cares about it.

Both are don't block from my side 👍 so go ahead and merge or add the RecordError and then merge.

@Jorropo
Copy link
Contributor Author

Jorropo commented Sep 4, 2023

I'm not too familiar with the otel API but in my linked PR for go-datastore I also added RecordError "to record err as an exception span event for this span". Whatever this means. Feel free to judge if that's needed or not (source).

Good question, I've always used SetStatus and it does what I need it to (show up red in the tracer and give me the error message).


Thx

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

Suggested version: v0.7.2

Comparing to: v0.7.1 (diff)

Changes in go.mod file(s):

diff --git a/go.mod b/go.mod
index 36c635a..e4249a2 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
 module github.com/libp2p/go-libp2p-routing-helpers
 
-go 1.19
+go 1.20
 
 require (
 	github.com/Jorropo/jsync v1.0.1

gorelease says:

# diagnostics
go.sum: one or more sums are missing. Run 'go mod tidy' to add missing sums.

# summary
Suggested version: v0.8.0

gocompat says:

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

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to master. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.

@Jorropo Jorropo merged commit 71d7c96 into master Sep 4, 2023
9 checks passed
@Jorropo Jorropo deleted the tracing branch September 4, 2023 14:00
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.

None yet

2 participants