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.

Sign up for GitHub

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

add-disk-buffering-debug-mode #664

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add-disk-buffering-debug-mode #664

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09b7047
add-disk-buffering-debug-mode
Annosha Oct 27, 2024
8773508
lint fixes
Annosha Oct 27, 2024
0976e0d
fixing-variables
Annosha Oct 27, 2024
e7ad0e5
Update core/src/main/java/io/opentelemetry/android/features/diskbuffe…
Annosha Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions ...main/java/io/opentelemetry/android/features/diskbuffering/DiskBufferingConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public final class DiskBufferingConfiguration {
private final long maxFileAgeForWriteMillis;
private final long minFileAgeForReadMillis;
private final long maxFileAgeForReadMillis;
private final boolean enableDebugMode;
Copy link
Member

Choose a reason for hiding this comment

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

I think a debug mode should be part of the OtelRumConfig or the lowest level of configuration (if there's one) so the whole SDK respects the debug mode flag, and not only for the disk buffering, this would be useful across the SDK and its configurations

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a case to be made for both. Top-level debug mode could imply disk buffering debug mode, but not the other way around. In any case, that should be a separate PR IMO. Feel free to log an issue to track.

Copy link
Member

Choose a reason for hiding this comment

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

All good, having an SDK debug mode for all options feels too much IMO, a top-level debug mode that logs everything everywhere and respects eg a severity log level for filtering would be cleaner and easier to enable/disable.
If we introduce a debug flag per feature eg diskbuffering, that just adds more complexity everywhere, I'd rather achieve this with TAG filtering if the global debug mode is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

I unresolved the comment so that I can copy-paste the comment URL otherwise the anchor link does not work, but it's not that you have to address this in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

#668

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I'm not entirely sure I made my point, so let's continue chatting about it at SIG sometime.


private DiskBufferingConfiguration(Builder builder) {
enabled = builder.enabled;
Expand All @@ -30,6 +31,7 @@ private DiskBufferingConfiguration(Builder builder) {
maxFileAgeForWriteMillis = builder.maxFileAgeForWriteMillis;
minFileAgeForReadMillis = builder.minFileAgeForReadMillis;
maxFileAgeForReadMillis = builder.maxFileAgeForReadMillis;
enableDebugMode = builder.enableDebugMode;
}

public static Builder builder() {
Expand Down Expand Up @@ -70,12 +72,30 @@ public static final class Builder {
private long maxFileAgeForWriteMillis = TimeUnit.SECONDS.toMillis(30);
private long minFileAgeForReadMillis = TimeUnit.SECONDS.toMillis(33);
private long maxFileAgeForReadMillis = TimeUnit.HOURS.toMillis(18);
private boolean enableDebugMode = false;

private ExportScheduleHandler exportScheduleHandler = DefaultExportScheduleHandler.create();

/** Sets the debug mode for disk buffering, enabling additional logging. */
public Builder setDebugMode(boolean enableDebugMode) {
this.enableDebugMode = enableDebugMode;
Logger.getLogger(DiskBufferingConfiguration.class.getName())
.log(
Level.INFO,
"Disk buffering has been "
+ (enabled ? "enabled." : "disabled.")
+ (enableDebugMode ? " Debug mode is active." : ""));
Comment on lines +84 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to log the enabled setting here -- that's a separate concern below. I recommend changing the log message to be simpler and only reflect the new value of enableDebugMode.

return this;
}

/** Enables or disables disk buffering. */
public Builder setEnabled(boolean enabled) {
this.enabled = enabled;
if (enableDebugMode) {
Logger.getLogger(DiskBufferingConfiguration.class.getName())
.log(Level.INFO, "Debug log message here");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this message is probably not what you intended?

}

return this;
}

Expand Down