-
Notifications
You must be signed in to change notification settings - Fork 76
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
[feat][sdk-105] Add telemetry #313
[feat][sdk-105] Add telemetry #313
Conversation
…hmap in constructor
@@ -794,7 +795,7 @@ public void debug(Throwable error, Map<String, Object> custom, String descriptio | |||
} | |||
|
|||
/** | |||
* Log an error at the level returned by {@link com.rollbar.notifier.Rollbar#level}. | |||
* Log an error at the level returned by {@link Level}. |
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.
The previous reference seem to be the correct one, since we are passing null
here for level which is going to be replaced down the line with a default by the previously referenced level
method.
.github/workflows/ci.yml
Outdated
if test "$GITHUB_REPOSITORY" = "rollbar/rollbar-java" -a "$GITHUB_BASE_REF" = ""; then | ||
openssl enc -aes-256-cbc -K "$SECRING_GPG_KEY" -iv "$SECRING_GPG_IV" -in "$ENCRYPTED_GPG_KEY_LOCATION" -out "$GPG_KEY_LOCATION" -d | ||
fi && | ||
./.github/scripts/release.sh | ||
|
||
- name: Cleanup Gradle cache | ||
run: | | ||
rm -f ~/.gradle/caches/modules-2/modules-2.lock | ||
rm -f ~/.gradle/caches/modules-2/gc.properties |
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.
Some of this stuff is not needed:
- We are signing with a new key using a different method than before. The GPG keyring file no longer exists.
- The conditions tested here are also part of the release script.
- Cache is managed by the
setup-java
action.
if test "$GITHUB_REPOSITORY" = "rollbar/rollbar-java" -a "$GITHUB_BASE_REF" = ""; then | |
openssl enc -aes-256-cbc -K "$SECRING_GPG_KEY" -iv "$SECRING_GPG_IV" -in "$ENCRYPTED_GPG_KEY_LOCATION" -out "$GPG_KEY_LOCATION" -d | |
fi && | |
./.github/scripts/release.sh | |
- name: Cleanup Gradle cache | |
run: | | |
rm -f ~/.gradle/caches/modules-2/modules-2.lock | |
rm -f ~/.gradle/caches/modules-2/gc.properties | |
./.github/scripts/release.sh |
import org.mockito.junit.MockitoJUnit; | ||
import org.mockito.junit.MockitoRule; | ||
|
||
public class RollbarBaseTest { |
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.
RollbarBaseTest
sounds like a base class for tests, but this doesn't seem to be one. Could you find a different name for it that reflects what is being tested 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.
the problem is that I'm testing the new methods in RollbarBase
🤔 RollbarBaseNotifierTest
?
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.
I'm testing the new methods
and nothing else, so it'd be great if the name of the test class reflected that it's a collection of telemetry related test cases.
rollbar-java/src/main/java/com/rollbar/notifier/config/ConfigBuilder.java
Show resolved
Hide resolved
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.
A few things to check again, but otherwise looking good 👍
* Set a {@link TelemetryEventTracker} implementation. | ||
* Default: {@link RollbarTelemetryEventTracker} with a {@link TimestampProvider}. | ||
* </p> | ||
* @param telemetryEventTracker true to enable truncation. |
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 looks like a copy/paste error.
* <p> | ||
* Maximum Telemetry Events sent in a payload. Default is | ||
* {@value RollbarTelemetryEventTracker#MAXIMUM_CAPACITY_FOR_TELEMETRY_EVENTS}. | ||
* </p> |
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.
Could you update the doc to explain that setting a custom tracker causes the value set here to be ignored?
* @param maximumTelemetryData max quantity of telemetry events sent. | ||
* @return the builder instance. | ||
*/ | ||
public ConfigBuilder maximumTelemetryData(int maximumTelemetryData) { |
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.
See comments on the other ConfigBuilder
.
* Set a {@link TelemetryEventTracker} implementation. | ||
* Default: {@link RollbarTelemetryEventTracker} with a {@link TimestampProvider}. | ||
* </p> | ||
* @param telemetryEventTracker true to enable truncation. |
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.
See comments on the other ConfigBuilder
.
Description of the change
This is adds support for the Telemetry feature (only given by user, automatic captures will be added later).
Overview
maximumTelemetryData
from the CommonConfig.TelemetryEventTracker
(in this case themaximumTelemetryData
from the CommonConfig would be ignored)Example
Modify default maximum capacity
Maximum capacity surpased
Override default TelemetryEventTracker
Usage
Type of change
Related issues
Checklists
Development
Code review