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

chore: add opentelemetry example #31

Closed

Conversation

vmihailenco
Copy link
Contributor

I will continue working on this, but it is a decent preview of how it will look.

@vmihailenco vmihailenco marked this pull request as draft November 7, 2022 13:09
@vmihailenco vmihailenco force-pushed the feat/opentelemetry-example branch from 474c12d to ceef58e Compare November 7, 2022 13:12
@vmihailenco
Copy link
Contributor Author

vmihailenco commented Nov 7, 2022

Regarding the instrumentation, I've noticed 2 things:

  • db.statement attribute is missing which is rather useful to have, for example, db.statement = "set key value"
  • looks like $redis->multi() and $redis->pipeline() require more work

@tillkruss I can try to address both issues, but wanted to check with you first.

@tillkruss
Copy link
Member

Good catch, feel free to add both.

The db.statement could be extremely massive. I read somewhere that values should be omitted, just keys should be logged.

@vmihailenco vmihailenco force-pushed the feat/opentelemetry-example branch from ceef58e to 1ac06f0 Compare November 9, 2022 12:10
@vmihailenco
Copy link
Contributor Author

vmihailenco commented Nov 9, 2022

I've sent #32 to record db.statement, but multi and pipeline look somewhat complicated so we probably should leave it for later. E.g. should we create a separate wrapper for mutli/exec? Or we can expect the state of existing Relay instance? Hard to tell without the source code...

I read somewhere that values should be omitted, just keys should be logged.

That recommendation only applies for span names. Attributes can be arbitrary large, for example, it is a common practice to record SQL queries which can be much larger than Redis commands.

@tillkruss
Copy link
Member

but multi and pipeline look somewhat complicated so we probably should leave it for later. E.g. should we create a separate wrapper for mutli/exec? Or we can expect the state of existing Relay instance? Hard to tell without the source code...

Relay works just like PhpRedis for transactions: https://github.com/phpredis/phpredis

@tillkruss
Copy link
Member

I've done the MULTI wrapping in Object Cache Pro, will take a look today how it could be handled.

@vmihailenco vmihailenco force-pushed the feat/opentelemetry-example branch from 1ac06f0 to 9c3ba46 Compare November 11, 2022 11:12
@vmihailenco vmihailenco deleted the feat/opentelemetry-example branch December 10, 2022 13:59
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