Skip to content

feat(RelayOpenTelemetry): add db.statement attribute#32

Closed
vmihailenco wants to merge 5 commits intocachewerk:mainfrom
vmihailenco:fix/db-statement
Closed

feat(RelayOpenTelemetry): add db.statement attribute#32
vmihailenco wants to merge 5 commits intocachewerk:mainfrom
vmihailenco:fix/db-statement

Conversation

@vmihailenco
Copy link
Copy Markdown
Contributor

There probably should be a way to disable this attribute in case the value is too large, but I am not sure how the API should look like so I left it for a future PR.

@vmihailenco vmihailenco force-pushed the fix/db-statement branch 2 times, most recently from 8c54ec6 to 3cb67e4 Compare November 9, 2022 12:41
Comment thread src/Psr/Tracing/RelayOpenTelemetry.php
@vmihailenco
Copy link
Copy Markdown
Contributor Author

PTAL. I've added db.statement attribute for *scan operation and transactions.

screen1
screen2

Comment on lines +120 to +121
->setAttribute('db.operation', $operation)
->setAttribute('db.statement', $stmt)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This won't work with __construct() and connect() and some other, which aren't Redis commands, but Relay specific methods.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"This" is db.statement? Why it does work for db.operation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's also sending invalid data currently. We're working on the stubs to make this easier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will make it easier to identify various types of methods: https://relay.so/api

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, sounds like I should wait for some input from you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@vmihailenco: We're exploring whether we can use 8.0 attributes in the class stubs. If so we could use reflection to check whether a method should be bypassed (setOption()), should be traced (connect()) or whether it's a Redis command (mget()).

Instead of maintaining arrays of functions we could parse all class methods once, or each method once when it's executed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for update. Let me know what changes do you expect here.

My 5 cents are that I would not worry about this. I'd say that tracing connect is the right thing to do and having setOption also would not hurt. Are you worried about having db.statement on such spans? Again, I don't see what real issues that causes.,,

@tillkruss
Copy link
Copy Markdown
Member

Closing in favor of 21f4e06.

@tillkruss tillkruss closed this Dec 10, 2022
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.

2 participants