-
Notifications
You must be signed in to change notification settings - Fork 664
[epilogue] Detailed trigger dependencies #8253
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
base: main
Are you sure you want to change the base?
[epilogue] Detailed trigger dependencies #8253
Conversation
This annotation allows users to mark logged fields and methods as dependent on other data points for analysis tools like AdvantageScope. This is particularly useful for complex conditions like triggers where understanding the component values is important for debugging. The annotation is @repeatable to allow multiple dependencies and targets both fields and methods. It includes comprehensive documentation with examples showing usage on trigger conditions. Addresses: wpilibsuite#8233
Adds @dependsOn to the supported annotation types list in the annotation processor, enabling the processor to discover and handle classes that use dependency metadata annotations. This establishes the foundation for processing @dependsOn annotations during the compilation phase. Addresses: wpilibsuite#8233
Adds comprehensive support for @dependsOn annotations in the logger generation process: - generateDependencyLogging() processes @dependsOn annotations on logged elements and generates additional logging calls for their dependencies - findDependencyElement() searches the class hierarchy for dependency elements by name, supporting both fields and methods - Dependency logging calls are generated alongside main element logging This enables analysis tools like AdvantageScope to automatically log related values when logging complex conditions, providing better debugging visibility without manual logging setup. The implementation handles: - Single and multiple @dependsOn annotations via @repeatable support - Field and method dependencies with proper name resolution - Class hierarchy traversal to find dependencies in superclasses - Integration with existing element handlers for consistent logging Addresses: wpilibsuite#8233
Adds comprehensive test coverage for the @dependsOn annotation functionality, verifying that: - The annotation processor correctly recognizes @dependsOn annotations - Multiple dependencies can be declared on a single element - Generated logger code includes logging calls for all declared dependencies - Dependencies are resolved correctly within the class The test uses a realistic scenario with position, target, and tolerance fields that are dependencies of an isAtTarget() method, matching the use case described in the GitHub issue. Addresses: wpilibsuite#8233
backend.log("position", object.position); | ||
backend.log("target", object.target); | ||
backend.log("tolerance", object.tolerance); |
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.
These extra logging calls are redundant, they'll just add extra overhead to the logger. I'd want to see these names in metadata for the isAtTarget
logging invocation - that'll mean updating EpilogueBackend to accept a metadata argument on all the log
methods, and update the NT and File backends to use that metadata
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.
These extra logging calls are redundant, they'll just add extra overhead to the logger. I'd want to see these names in metadata for the
isAtTarget
logging invocation - that'll mean updating EpilogueBackend to accept a metadata argument on all thelog
methods, and update the NT and File backends to use that metadata
Sounds good. I made the changes.
// For each dependency, find the corresponding element and generate logging | ||
for (DependsOn dependency : dependencies) { | ||
String dependencyName = dependency.value(); | ||
Element dependencyElement = findDependencyElement(clazz, dependencyName); |
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.
You can just look up elements in the fieldsToLog
and methodsToLog
lists, no need to walk the class hierarchy again
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.
You can just look up elements in the
fieldsToLog
andmethodsToLog
lists, no need to walk the class hierarchy again
Thanks!
Here is how I do it now
// Precompute set of logged names for efficient lookup
Set<String> loggedNames = Stream.concat(fieldsToLog.stream(), methodsToLog.stream())
.map(ElementHandler::loggedName)
.collect(Collectors.toSet());
// Collect valid dependency names by checking against logged elements
List<String> validDependencyNames = new ArrayList<>();
for (DependsOn dependency : dependencies) {
String dependencyName = dependency.value();
if (loggedNames.contains(dependencyName)) {
validDependencyNames.add(dependencyName);
}
}
backend.log("position", object.position); | ||
backend.log("target", object.target); | ||
backend.log("tolerance", object.tolerance); | ||
backend.log("isAtTarget", object.isAtTarget()); |
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 generated call should be something like backend.log("isAtTarget", object.isAtTarget, new LogMetadata(List.of("position", "target", "tolerance")));
. A lambda-based approach to cut down on redundant object allocations would also work; metadata won't change so there's no point to specify it on every call
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 don't think a lambda-based approach (assuming you mean passing a lambda to the method to create the LogMetadata
instead of constructing a new LogMetadata
every time) would help with redundant object allocations, since the lambda object still gets allocated every time. (After all, #8190 explicitly removed lambda usage to avoid allocating the lambda objects) I think the only solutions to avoid redundant object allocations are either a) caching the LogMetadata
object or b) checking somehow if this is the first log call to determine if the metadata needs to be provided.
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.
@KangarooKoala Thanks for the elaboration! I followed through with a:
caching the
LogMetadata
object
// Static field declaration
private static final LogMetadata $metadata_..._isAtTarget;
// Static initializer
static {
$metadata_..._isAtTarget = new LogMetadata(List.of("position", "target", "tolerance"));
}
// Usage (reuses cached object)
backend.log("isAtTarget", object.isAtTarget(), $metadata_..._isAtTarget);
I also updated tests in /AnnotationProcessorTest.java
to match new static field caching approach instead of inline construction
Introduces LogMetadata to provide context about dependencies between logged data points, enabling better analysis in tools like AdvantageScope.
Adds overloaded log methods that accept LogMetadata parameter for dependency tracking. Default implementations delegate to existing methods for backward compatibility.
Refactored to generate LogMetadata for @dependsOn annotations, replacing individual dependency logging with consolidated metadata approach. Conditionally imports LogMetadata only when needed.
Modified annotation processor test to expect LogMetadata import and consolidated dependency logging in generated logger classes.
…in LoggerGenerator and update test to use cached LogMetadata
…s for improved dependency tracking
.map(name -> "\"" + name + "\"") | ||
.collect(java.util.stream.Collectors.joining(", ")); | ||
|
||
return "new edu.wpi.first.epilogue.logging.LogMetadata(java.util.List.of(" + dependencyList + "))"; |
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.
Let's actually store metadata as fields in the generated loggers, eg:
class ExampleLogger extends ClassSpecificLogger<Example> {
private final LogMetadata fooMetadata = new LogMetadata(List.of("x", "y", "z"));
@Override
public void update(EpilogueBackend backend, Example object) {
if (Epilogue.shouldLog(Logged.Importance.DEBUG)) {
backend.log("x", object.x);
backend.log("y", object.y);
backend.log("z", object.z);
backend.log("foo", object.foo, fooMetadata);
}
}
}
This would require two generation steps: one to create the fields, and another to reference them in the logging calls.
out.println(dependencyInvocation.indent(6).stripTrailing() + ";"); | ||
var metadata = generateLogMetadata(loggableElement, clazz); | ||
if (metadata != null) { | ||
var modifiedInvocation = logInvocation.replaceFirst("\\)$", ", " + metadata + ")"); |
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.
It'd be better to have the element handler generate the metadata arguments instead of manipulating the return values after the fact; it can be tricky to handle all possible cases with string manipulation.
Splitting the metadata to be declared as a field also means the element handler can just emit element.getSimpleName() + "Metadata"
without needing to know about anything the metadata depends upon
* or relationships with other logged values. This is particularly useful for analysis | ||
* tools like AdvantageScope to understand complex conditions and triggers. | ||
*/ | ||
public final class LogMetadata { |
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 could be a record. Additionally, declaring a default empty metadata object (eg LogMetadata.kEmpty
or some such) would be useful for logging that doesn't have metadata
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.
Sounds good
public record LogMetadata(List<String> dependencies) {
public static final LogMetadata kEmpty = new LogMetadata(List.of());
default void log(String identifier, int value, LogMetadata metadata) { | ||
log(identifier, value); | ||
} |
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.
These should be flipped: log(identifier, value)
should delegate to log(identifier, value, LogMetadata.kEmpty)
You will also need to implement these methods for NTEpilogueBackend
and FileBackend
and tweak those classes so they'll use the metadata that's passed in. (This code is mostly for example; improvements could certainly be made to reduce code duplication and unnecessary setProperties
/setMetadata
calls when the log metadata is empty)
// NTEpilogueBackend.java
private static <T extends Topic> T topicWithMetadata(T topic, LogMetadata metadata) {
var dependenciesJson = metadata.dependencies().stream().map(d -> '"' + d + '"').join(Collectors.joining(", ", "[", "]"));
String metadataJson = """
{ "dependencies": %s }
""".trim().formatted(metadataJson);
topic.setProperties(metadataJson);
return topic;
}
@Override
public void log(String identifier, int value, LogMetadata metadata) {
if (m_publishers.containsKey(identifier)) {
((IntegerPublisher) m_publishers.get(identifier)).set(value);
} else {
var publisher = topicWithMetadata(m_nt.getIntegerTopic(identifier), dependencies).publish();
m_publishers.put(identifier, publisher);
publisher.set(value);
}
}
// ... etc for other logging methods
// FileBackend.java
@SuppressWarnings("unchecked")
private <E extends DataLogEntry> E getEntry(String identifier, BiFunction<DataLog, String, ? extends E> ctor, LogMetadata metadata) {
if (m_entries.get(identifier) != null) {
return (E) m_entries.get(identifier);
}
var entry = ctor.apply(m_dataLog, identifier);
var dependenciesJson = metadata.dependencies().stream().map(d -> '"' + d + '"').join(Collectors.joining(", ", "[", "]"));
String metadataJson = """
{ "dependencies": %s }
""".trim().formatted(metadataJson);
entry.setMetadata(metadataJson);
m_entries.put(identifier, entry);
return entry;
}
@Override
public void log(String identifier, int value, LogMetadata metadata) {
getEntry(identifier, IntegerLogEntry::new, metadata).append(value);
}
// ... etc for other logging methods
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.
These should be flipped:
log(identifier, value)
should delegate tolog(identifier, value, LogMetadata.kEmpty)
You will also need to implement these methods for
NTEpilogueBackend
andFileBackend
and tweak those classes so they'll use the metadata that's passed in.
Understood.
I changed all abstract methods to default methods that delegate to metadata versions. Heres an example:
before:
void log(String identifier, int value);
after:
default void log(String identifier, int value) {
log(identifier, value, LogMetadata.kEmpty);
}
private static final LogMetadata $metadata_edu_wpi_first_epilogue_Example_isAtTarget; | ||
static { | ||
$metadata_edu_wpi_first_epilogue_Example_isAtTarget = new edu.wpi.first.epilogue.logging.LogMetadata(java.util.List.of("position", "target", "tolerance")); |
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.
Let's have imports for these types. Even though the file is generated, it'd be nice to be more human-friendly
*/ | ||
public static String logMetadataFieldName(Element element) { | ||
return "$metadata_%s_%s" | ||
.formatted(element.getEnclosingElement().toString().replace(".", "_"), element.getSimpleName()); |
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.
How will this behave if a logged field and a logged method have the same simple name?
@Logged
class Example {
@Logged
int foo = 0;
@Logged
public int foo() { return 42; }
}
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.
It compiles right now, but that's a good question. Should I try making it disambiguous? Maybe we could append suffixes?
/** | ||
* Finds an element (field or method) in a class by name. | ||
* | ||
* @param clazz the class to search in | ||
* @param name the name of the element to find (method names should include parentheses) | ||
* @return the found element, or null if not found | ||
*/ |
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 doc comment without a method should get cleaned up.
out.printf( | ||
" // Cached LogMetadata for element %s with @DependsOn annotations%n", | ||
element.getSimpleName()); | ||
out.printf(" private static final LogMetadata %s;%n", logMetadataFieldName(element)); |
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 can't we initialize the LogMetadata
caches inline with the field declarations? (e.g., private static final LogMetadata $metadata_edu_wpi_first_epilogue_Example_isAtTarget = new LogMetadata(List.of("position", "target", "tolerance"));
)
This would mean that we can remove the code in the static {}
initializer block, which would greater simplify the logic.
// Generate log invocation for the main element | ||
var metadata = generateLogMetadata(loggableElement, loggableFields, loggableMethods); | ||
if (metadata != null) { | ||
var modifiedInvocation = logInvocation.replaceFirst("\\)$", java.util.regex.Matcher.quoteReplacement(", " + metadata + ")")); |
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.
Can we import Matcher
here?
* @param useStaticField if true, return static field reference; if false, return inline construction | ||
* @return LogMetadata construction code or field reference, or null if no valid dependencies | ||
*/ | ||
private String generateLogMetadata(Element element, List<VariableElement> fieldsToLog, List<ExecutableElement> methodsToLog, boolean useStaticField) { |
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.
Instead of a single method with very different return values based on a boolean flag (which is hard to remember which is which), would it be better to refactor this into a collectLogDependencies()
method (returning dependencies
from the current version of the code) and a generateLogMetadata()
method (using collectLogDependencies()
and not using useStaticField
)?
…logueBackend and update LoggerGenerator to use instance fields for LoggerGenerator
public static String logMetadataFieldName(Element element) { | ||
return "$metadata_%s_%s" | ||
.formatted(element.getEnclosingElement().toString().replace(".", "_"), element.getSimpleName()); | ||
String suffix = element instanceof javax.lang.model.element.VariableElement ? "_field" : "_method"; |
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.
Use imports instead of fully qualified names, please
" // Cached LogMetadata for element %s with @DependsOn annotations%n", | ||
element.getSimpleName()); | ||
out.printf(" private static final LogMetadata %s;%n", logMetadataFieldName(element)); | ||
out.printf(" private final LogMetadata %s;%n", logMetadataFieldName(element)); |
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.
These fields could still be static, just do the initialization inline instead of in the constructor.
var modifiedInvocation = | ||
logInvocation.replaceFirst( | ||
"\\)$", | ||
java.util.regex.Matcher.quoteReplacement(", " + metadata + ")")); |
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.
FQN should be an import
String dependencyList = | ||
validDependencyNames.stream() | ||
.map(name -> "\"" + name + "\"") | ||
.collect(java.util.stream.Collectors.joining(", ")); |
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.
FQN should be an import
@SuppressWarnings("PMD.UnnecessaryCastRule") | ||
public void log(String identifier, int[] value) { | ||
long[] widened = new long[value.length]; | ||
for (int i = 0; i < value.length; i++) { | ||
widened[i] = (long) value[i]; | ||
} | ||
java.util.Arrays.setAll(widened, i -> value[i]); |
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.
These changes are unrelated and should be reverted
|
||
if (!m_entries.containsKey(identifier)) { | ||
var entry = ProtobufLogEntry.create(m_dataLog, identifier, proto); | ||
if (!metadata.dependencies().isEmpty()) { |
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.
Let's have a function on LogMetadata
to check if it's empty or not. For now, it'd just look at the dependencies list, but could be changed in the future if more types of metadata are added
if (!metadata.dependencies().isEmpty()) { | ||
var dependenciesJson = | ||
metadata.dependencies().stream() | ||
.map(d -> '"' + d + '"') | ||
.collect(Collectors.joining(", ", "[", "]")); | ||
String metadataJson = | ||
""" | ||
{ "dependencies": %s } | ||
""" | ||
.trim() | ||
.formatted(dependenciesJson); |
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 JSON generation is duplicated in the NT backend, too. Let's move it to a LogMetadata.toJson()
method so it's reusable.
if (!metadata.dependencies().isEmpty()) { | ||
var dependenciesJson = | ||
metadata.dependencies().stream() | ||
.map(d -> '"' + d + '"') | ||
.collect(Collectors.joining(", ", "[", "]")); | ||
String metadataJson = | ||
""" | ||
{ "dependencies": %s } | ||
""" | ||
.trim() | ||
.formatted(dependenciesJson); | ||
entry.setMetadata(metadataJson); |
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 duplicated JSON code could be DRYed up
} | ||
|
||
// Helper method for applying metadata to topics | ||
private void applyMetadata(edu.wpi.first.networktables.Topic topic, LogMetadata metadata) { |
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.
FQN should be imported
long[] widened = new long[value.length]; | ||
java.util.Arrays.setAll(widened, i -> value[i]); |
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.
Use the original widening code, it's more efficient
…related methods update LogMetadata with isEmpty and toJson methods for better metadata handling
out.println(" try {"); | ||
|
||
out.println(" var rootLookup = MethodHandles.lookup();"); | ||
if (requiresVarHandles) { | ||
out.println(" try {"); | ||
out.println(" var rootLookup = MethodHandles.lookup();"); | ||
} |
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 if
guard is no longer necessary since now requiresVarHandles
is the only condition of the outer if-statement.
" throw new RuntimeException(" | ||
+ "\"[EPILOGUE] Could not load private fields for logging!\", e);"); | ||
out.println(" }"); | ||
if (requiresVarHandles) { |
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.
Same as above.
epilogue-processor/src/main/java/edu/wpi/first/epilogue/processor/LoggerGenerator.java
Show resolved
Hide resolved
if (loggedNames.contains(dependencyName)) { | ||
validDependencyNames.add(dependencyName); | ||
} |
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.
Is silently skipping dependency names that don't match any logged names the best behavior? Should we add tests for this?
* @param methodsToLog the list of methods that will be logged | ||
* @return List of valid dependency names, or null if no valid dependencies | ||
*/ | ||
private List<String> collectLogDependencies( |
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.
Maybe I'm missing something, but can't this be static?
private String generateLogMetadata( | ||
Element element, | ||
List<VariableElement> fieldsToLog, | ||
List<ExecutableElement> methodsToLog, | ||
boolean useStaticField) { |
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.
Instead of a boolean
parameter, it'd be more readable at the call-site to define methods with two different names. (e.g., generateLogMetadataFieldReference()
and generateLogMetadataConstruction()
. These are just examples though (and not the best)- If you think a different name would be better, go ahead and try it out!)
ec7a461
to
79b132c
Compare
… generated LogMetadata accordingly. Also add tests for handling invalid dependency names.
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 someone mark old conversations as resolved? (I don't have GitHub permissions to, since I'm neither the author nor someone with write access)
out.println(); | ||
} | ||
|
||
// Static initializer block to load VarHandles only |
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.
Did anything change so that this no longer generates reflection fields (which were mentioned in the original version of the comment)?
public String toJson() { | ||
var dependenciesJson = | ||
dependencies.stream() | ||
.map(d -> '"' + d + '"') |
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.
Is it necessary to add logic to address dependencies containing double quotes? It should never happen, so maybe not, but it's still worth considering.
Add
@DependsOn
annotation to hopefully resolve the following issue: #8233@DependsOn
is on the Epilogue logging framework, allowing fields and methods marked with@Logged
to declare dependencies on other data points in the same class.The changes include annotation processor updates, logger code generation updates, and new tests to verify the behavior.
Let me know what you all think!