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

Opt-in core-to-REST-server instrumentation #3432

Merged
merged 8 commits into from
Aug 11, 2022

Conversation

johnkerl
Copy link
Contributor

@johnkerl johnkerl commented Aug 7, 2022

Provides crucial information on core-to-REST-server HTTP operations. This is essential for analyzing and minimizing remote-request latencies. An indispensable counterpart to Jaeger tracing, while Jaeger tracing isn't enough to give us a full picture on all interactions in all contexts. Having this in-core will make these performance numbers readily accessible where they're most needed, namely, in notebook environments.


TYPE: IMPROVEMENT
DESC: Opt-in core-to-REST-server instrumentation

tiledb/common/logger.cc Outdated Show resolved Hide resolved
tiledb/common/logger.h Outdated Show resolved Hide resolved
@johnkerl johnkerl force-pushed the kerl/core-to-rest-instrumentation branch from 7c727be to c8ec5ac Compare August 8, 2022 00:00
@johnkerl johnkerl force-pushed the kerl/core-to-rest-instrumentation branch from e3bd25a to c3cdd37 Compare August 9, 2022 03:34
Copy link
Member

@ypatia ypatia left a comment

Choose a reason for hiding this comment

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

Thanks for the good work! I added a few comments, I will push a new version to address them if you don't mind to save you the hustle, together with a small ordering fix in the config unit-tests that will make CI pass. HTH

test/src/unit-capi-config.cc Outdated Show resolved Hide resolved
tiledb/sm/c_api/tiledb.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/config.h Outdated Show resolved Hide resolved
tiledb/sm/rest/curl.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/curl.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/curl.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/curl.cc Outdated Show resolved Hide resolved
@ypatia ypatia force-pushed the kerl/core-to-rest-instrumentation branch from f5ccd91 to 06487d3 Compare August 9, 2022 10:59
@johnkerl
Copy link
Contributor Author

johnkerl commented Aug 9, 2022

@ypatia is there anything else to do on this PR? If not can we approve it?

@ypatia ypatia requested a review from Shelnutt2 August 9, 2022 13:11
@ypatia
Copy link
Member

ypatia commented Aug 9, 2022

@ypatia is there anything else to do on this PR? If not can we approve it?

It's ok for me, but let's have an approval from either @Shelnutt2 or @ihnorton before we merge

@ypatia
Copy link
Member

ypatia commented Aug 10, 2022

As per @Shelnutt2's initial review we have:

  • Removed flush()
  • Removed the config variable all along as it was only needed to disable flush() if that would bring perf degradation. Now only trace level logging level is needed.
  • Replaced LOG_TRACE() macro w/ using the structured logger_ member of Curl class to give finer debug info on what query is executing.

@ypatia ypatia force-pushed the kerl/core-to-rest-instrumentation branch from 0bc196b to 8eaa0ba Compare August 10, 2022 09:38
@Shelnutt2 Shelnutt2 merged commit 2b3358e into dev Aug 11, 2022
@Shelnutt2 Shelnutt2 deleted the kerl/core-to-rest-instrumentation branch August 11, 2022 15:44
github-actions bot pushed a commit that referenced this pull request Aug 11, 2022
* Opt-in core-to-REST-server instrumentation

* clang format

* add flushing for real-time greppability

* fix formatting

* code-review feedback

* Few improvements based on commenst + CI fix

* remove flush which supported pipe-to-grep use-case

* Address @Shelnutt2 initial review comments

Co-authored-by: Ypatia Tsavliri <ypatia@tiledb.com>
Shelnutt2 pushed a commit that referenced this pull request Aug 11, 2022
* Opt-in core-to-REST-server instrumentation

* clang format

* add flushing for real-time greppability

* fix formatting

* code-review feedback

* Few improvements based on commenst + CI fix

* remove flush which supported pipe-to-grep use-case

* Address @Shelnutt2 initial review comments

Co-authored-by: Ypatia Tsavliri <ypatia@tiledb.com>

Co-authored-by: John Kerl <kerl.john.r@gmail.com>
Co-authored-by: Ypatia Tsavliri <ypatia@tiledb.com>
@johnkerl
Copy link
Contributor Author

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants