You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "klsince (via GitHub)" <gi...@apache.org> on 2023/06/26 20:38:58 UTC
[GitHub] [pinot] klsince opened a new pull request, #10977: allow to add custom context into query trace
klsince opened a new pull request, #10977:
URL: https://github.com/apache/pinot/pull/10977
Allow custom context to be added into query trace and returned as part of the TraceInfo field in query response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince merged pull request #10977: allow to add custom context into query trace
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince merged PR #10977:
URL: https://github.com/apache/pinot/pull/10977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10977: allow to add custom context into query trace
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10977:
URL: https://github.com/apache/pinot/pull/10977#issuecomment-1608287569
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10977?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10977](https://app.codecov.io/gh/apache/pinot/pull/10977?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2f82064) into [master](https://app.codecov.io/gh/apache/pinot/commit/8170d435c5e8999daae9bb393e9d172aea2bdbd7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8170d43) will **decrease** coverage by `0.12%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10977 +/- ##
==========================================
- Coverage 0.11% 0.00% -0.12%
==========================================
Files 2189 1655 -534
Lines 118056 86750 -31306
Branches 17872 13682 -4190
==========================================
- Hits 137 0 -137
+ Misses 117899 86750 -31149
+ Partials 20 0 -20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `0.00% <0.00%> (ø)` | |
| unittests1temurin17 | `0.00% <0.00%> (ø)` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `?` | |
| unittests2temurin20 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/apache/pinot/core/util/trace/BuiltInTracer.java](https://app.codecov.io/gh/apache/pinot/pull/10977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL3RyYWNlL0J1aWx0SW5UcmFjZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...rc/main/java/org/apache/pinot/spi/trace/Scope.java](https://app.codecov.io/gh/apache/pinot/pull/10977?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvU2NvcGUuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [535 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10977/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gortiz commented on a diff in pull request #10977: allow to add custom context into query trace
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10977:
URL: https://github.com/apache/pinot/pull/10977#discussion_r1244761724
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/BuiltInTracer.java:
##########
@@ -50,6 +50,15 @@ public void close() {
}
TraceContext.logTime(operatorName, duration);
}
+
+ @Override
+ public void close(Object context) {
+ String operatorName = _operator.getSimpleName();
+ if (LOGGER.isTraceEnabled()) {
Review Comment:
The rule is (assuming `string` and `concatenation` are actual variables decided in runtime):
- Most expensive option: `Logger.trace("some " + string + " with " + concatenation)` calls toString on the variables and concatenates the strings (which is quite expensive in terms of allocating and moving) each time we evaluate that, even when trace is disabled
- Second most expensive option:
- `Logger.trace("some {} string with {}", string, concatenation)` is a syntactic sugar of `Logger.trace("some {} string with {}", new String[string, concatenation])`, which creates an array each time we evaluate the line, even if trace is disabled. This construction does not eagerly call toString on the variables and just create an array of pointers, which is around 32 bytes per variable we use. It also doesn't need to concatenate it, so it is cheaper in terms of moving memory
- Log4J2 has also a construction like: `Logger.trace(() -> "some " + string + " with " + concatenation)`. This is similar to the previous option: It allocates a lambda instead of an array but does not call toString and may be easier to read.
- Cheapest option:
- `if (LOGGER.isTraceEnabled()) { `Logger.trace("some " + string + " with " + concatenation)`;}`. Given that we only execute LOGGER.trace(...) if LOGGER.isTraceEnabled() is true, then we only evalute the string is we are going to actually print it somewhere.
This applies not only to trace, but to all other logging methods. But trace is almost never actually printed, while error, warn are almost always printed and info is printed very often. Therefore we should always try to use this techniques in the hotpath, but the actual benefit is gonna be there in debug or tracer cases.
BTW, it is important to note that this only matters when we concatenate actual variables. Things like:
```
String literal = "world"
Logger.trace("hello " + literal)
```
or
```
Logger.trace("this is a very long sentence " +
"so I'm going to write it in two lines")
```
Are fine as Java should optimize them. In the second case the optimization will be done for sure by javac, so we can even verify that with javah (which shows the bytecode of a .class). In the first case I'm not sure whether javac will optimize that, but the JIT should optimize it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10977: allow to add custom context into query trace
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10977:
URL: https://github.com/apache/pinot/pull/10977#discussion_r1244334712
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/BuiltInTracer.java:
##########
@@ -50,6 +50,15 @@ public void close() {
}
TraceContext.logTime(operatorName, duration);
}
+
+ @Override
+ public void close(Object context) {
+ String operatorName = _operator.getSimpleName();
+ if (LOGGER.isTraceEnabled()) {
Review Comment:
sry, missed this comment. I did this same way as in the close() method above.
I think it's unnecessary if not on very critical path, as only reference is passed to trace() method. But this simple if-check may save cost of some method calls, considering this span.close() is on query critical path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] klsince commented on a diff in pull request #10977: allow to add custom context into query trace
Posted by "klsince (via GitHub)" <gi...@apache.org>.
klsince commented on code in PR #10977:
URL: https://github.com/apache/pinot/pull/10977#discussion_r1244334712
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/BuiltInTracer.java:
##########
@@ -50,6 +50,15 @@ public void close() {
}
TraceContext.logTime(operatorName, duration);
}
+
+ @Override
+ public void close(Object context) {
+ String operatorName = _operator.getSimpleName();
+ if (LOGGER.isTraceEnabled()) {
Review Comment:
sry, missed this comment. I did this same way as in the close() method above.
I think it's unnecessary if not on very critical path, as only reference is passed to trace() method. But this simple if-check may save cost of some method calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10977: allow to add custom context into query trace
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10977:
URL: https://github.com/apache/pinot/pull/10977#discussion_r1244329566
##########
pinot-core/src/main/java/org/apache/pinot/core/util/trace/BuiltInTracer.java:
##########
@@ -50,6 +50,15 @@ public void close() {
}
TraceContext.logTime(operatorName, duration);
}
+
+ @Override
+ public void close(Object context) {
+ String operatorName = _operator.getSimpleName();
+ if (LOGGER.isTraceEnabled()) {
Review Comment:
guessing this check is here to avoid unnecessary string concat? this is only needed if we pass in a function to trace right? for this format it is unnecessary? (CC @gortiz since we had similar discussion previously)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org