-
Notifications
You must be signed in to change notification settings - Fork 866
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
Update Telemetry env variable. #2356
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2356 +/- ##
==========================================
- Coverage 72.01% 71.92% -0.09%
==========================================
Files 78 78
Lines 3648 3648
Branches 58 58
==========================================
- Hits 2627 2624 -3
- Misses 1017 1020 +3
Partials 4 4 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
if (java.lang.System.getenv("SM_TELEMETRY_LOG_REV_2022_12") != null) { | ||
loggerTelemetryMetrics.info("Telemetry enabled."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this log is needed at here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logging message will help us to get endpoint name using TS for query total invoke account (in the case of no error log being generated.)
@@ -283,15 +285,15 @@ public void run() { | |||
} catch (OutOfMemoryError oom) { | |||
logger.error("Out of memory error when creating workers", oom); | |||
status = HttpURLConnection.HTTP_ENTITY_TOO_LARGE; | |||
if (java.lang.System.getenv("SM_TELEMETRY_LOG") != null) { | |||
if (java.lang.System.getenv("SM_TELEMETRY_LOG_REV_2022_12") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need get and check this env var everywhere? It only needs once and simply flag can be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
loggerTelemetryMetrics.info( | ||
"ModelServerError.Count:1|#TorchServe:{},{}:-1", | ||
ConfigManager.getInstance().getVersion(), | ||
oom.getClass().getCanonicalName()); | ||
} | ||
} catch (Throwable t) { | ||
logger.warn("Backend worker thread exception.", t); | ||
if (java.lang.System.getenv("SM_TELEMETRY_LOG") != null) { | ||
if (java.lang.System.getenv("SM_TELEMETRY_LOG_REV_2022_12") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a quick clarification regarding how this env variable is being used
This reverts commit 7270447.
Description
Please read our CONTRIBUTING.md prior to creating your first pull request.
Please include a summary of the feature or issue being fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #(issue)
Type of change
Please delete options that are not relevant.
Feature/Issue validation/testing
Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Test A
Logs for Test A
Test B
Logs for Test B
Checklist: