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

feat(tracing): improved tracing for persisters and requests #878

Merged
merged 4 commits into from
May 31, 2022

Conversation

icyphox
Copy link
Contributor

@icyphox icyphox commented May 25, 2022

This patch introduces tracing for persisters and requests.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@icyphox icyphox requested a review from zepatrik as a code owner May 25, 2022 07:16
@hperl hperl self-requested a review May 27, 2022 07:55
@@ -41,6 +41,9 @@ func (RelationTuple) TableName(_ context.Context) string {
}

func (r *RelationTuple) toInternal(ctx context.Context, nm namespace.Manager, p *Persister) (*relationtuple.InternalRelationTuple, error) {
ctx, span := p.d.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.toInternal")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that we are repeating the package and function name in this and other functions. I wonder if it would make sense to ask the runtime for that information instead, so that this information does not get out-of-sync in case we rename or move the functions. WDYT?

func spanNameOfCurrentFunction() string {
	pc, _, _, ok := runtime.Caller(0)
	if !ok {
		return "unknown"
	}
	if f := runtime.FuncForPC(pc); f != nil {
		return f.Name()
	}
	return "unknown"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nifty, but am I using it correctly? (Looks to be returning its own name): https://go.dev/play/p/Aa0yIFb3cjq
I remember experimenting with this idea previously, this was my PoC:

func spanName() {
	pc := make([]uintptr, 15)
	n := runtime.Callers(2, pc)
	frames := runtime.CallersFrames(pc[:n])
	frame, _ := frames.Next()
	fmt.Println(frame.Function)
}

// Prints:
// module/package.Foo

Perhaps I'll create a separate PR for it (should be in ory/x anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it should be runtime.Caller(1) in your example.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, just some minor questions.
Is there maybe a linter we can use to ensure the span name and function name are consistent?

internal/driver/registry_default.go Outdated Show resolved Hide resolved
internal/driver/daemon.go Outdated Show resolved Hide resolved
internal/driver/daemon.go Outdated Show resolved Hide resolved
@zepatrik zepatrik merged commit eb62c50 into master May 31, 2022
@zepatrik zepatrik deleted the feat/tracing branch May 31, 2022 10:17
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

3 participants