Skip to content

Conversation

@sfc-gh-ljoswiak
Copy link
Collaborator

Changes in this PR:

  • Removes a/ from tracing namespace in special-key-space. For example, \xff\xff/tracing/a/transaction_id becomes \xff\xff/tracing/transaction_id.

Style

  • All variable and function names make sense.
  • The code is properly formatted (consider running git clang-format).

Performance

  • All CPU-hot paths are well optimized.
  • The proper containers are used (for example std::vector vs VectorRef).
  • There are no new known SlowTask traces.

Testing

  • The code was sufficiently tested in simulation.
  • If there are new parameters or knobs, different values are tested in simulation.
  • ASSERT, ASSERT_WE_THINK, and TEST macros are added in appropriate places.
  • Unit tests were added for new algorithms and data structure that make sense to unit-test
  • If this is a bugfix: there is a test that can easily reproduce the bug.

@sfc-gh-clin
Copy link
Collaborator

LGTM.
But I may have a question that if you will add more to this \xff\xff/tracing in the future?
If so, maybe adding a nested prefix here is better, like \xff\xff/tracing/<some_prefix>.
In the future, if you have a different read logic on a range \xff\xff/tracing/<future_prefix>, it is easy to add.
If you think this is enough, then it is good to use \xff\xff/tracing

@sfc-gh-ljoswiak
Copy link
Collaborator Author

LGTM.
But I may have a question that if you will add more to this \xff\xff/tracing in the future?
If so, maybe adding a nested prefix here is better, like \xff\xff/tracing/<some_prefix>.
In the future, if you have a different read logic on a range \xff\xff/tracing/<future_prefix>, it is easy to add.
If you think this is enough, then it is good to use \xff\xff/tracing

I don't think there will be more keys added to the top level tracing/ layer, as the only control clients need to have over tracing is to set a custom identifier or to disable tracing for the transaction. Maybe there will be support for an action like reading a log specific to a span, but in that case the request would look like get \xff\xff/tracing/<span-id>/logs, reading data specific to a span. I guess if that occurred, it could change the range read behavior of reading the range \xff\xff/tracing/ - \xff\xff/tracing0?

@sfc-gh-clin
Copy link
Collaborator

sfc-gh-clin commented Dec 9, 2020

LGTM.
But I may have a question that if you will add more to this \xff\xff/tracing in the future?
If so, maybe adding a nested prefix here is better, like \xff\xff/tracing/<some_prefix>.
In the future, if you have a different read logic on a range \xff\xff/tracing/<future_prefix>, it is easy to add.
If you think this is enough, then it is good to use \xff\xff/tracing

I don't think there will be more keys added to the top level tracing/ layer, as the only control clients need to have over tracing is to set a custom identifier or to disable tracing for the transaction. Maybe there will be support for an action like reading a log specific to a span, but in that case the request would look like get \xff\xff/tracing/<span-id>/logs, reading data specific to a span. I guess if that occurred, it could change the range read behavior of reading the range \xff\xff/tracing/ - \xff\xff/tracing0?

yeah, adding \xff\xff/tracing/<span-id>/logs, sounds like a behavior change(an API change), which I think if you want to add it, you may need to wait until the next major version release(if the current major version has already been used) and update the api notes. Is it okay?

@sfc-gh-ljoswiak
Copy link
Collaborator Author

I'm speculating about possible future features -- however, in general I think most span state should be internal and the transaction shouldn't need to access it. In that case, the above example wouldn't need to be added. @sfc-gh-mpilman do you have thoughts on this?

@sfc-gh-clin
Copy link
Collaborator

sfc-gh-clin commented Dec 9, 2020

Sorry for misleading, I just confirm the contract with Andrew for what is a behavior change in special keys.
So adding anything new inside a module(a.k.a. \xff\xff/tracing) is considering a behavior change.
So anything you plan to add to the tracing module in the future will be a behavior change and need a major release for it.
Thus, I think adding a top-tier prefix doesn't help anything here.
(I was taking it wrong as adding non-overlapping ranges are not considered API change.)

@jzhou77 jzhou77 merged commit 0ecfaf1 into apple:master Dec 21, 2020
@sfc-gh-ljoswiak sfc-gh-ljoswiak deleted the fixes/sps-tracing branch January 26, 2021 17:40
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.

3 participants