diff --git a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/Chunk.java b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/Chunk.java index d49e13477..e41717919 100644 --- a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/Chunk.java +++ b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/Chunk.java @@ -93,10 +93,6 @@ private void writeBuiltinType(LEB128Writer writer, TypedValueImpl typedValue) { throw new IllegalArgumentException(); } - if (value == null && builtin != TypesImpl.Builtin.STRING) { - // skip the non-string built-in values - return; - } switch (builtin) { case STRING: { if (value == null) { @@ -114,35 +110,35 @@ private void writeBuiltinType(LEB128Writer writer, TypedValueImpl typedValue) { break; } case BYTE: { - writer.writeByte((byte) value); + writer.writeByte(value == null ? (byte) 0 : (byte) value); break; } case CHAR: { - writer.writeChar((char) value); + writer.writeChar(value == null ? (char) 0 : (char) value); break; } case SHORT: { - writer.writeShort((short) value); + writer.writeShort(value == null ? (short) 0 : (short) value); break; } case INT: { - writer.writeInt((int) value); + writer.writeInt(value == null ? 0 : (int) value); break; } case LONG: { - writer.writeLong((long) value); + writer.writeLong(value == null ? 0L : (long) value); break; } case FLOAT: { - writer.writeFloat((float) value); + writer.writeFloat(value == null ? 0.0f : (float) value); break; } case DOUBLE: { - writer.writeDouble((double) value); + writer.writeDouble(value == null ? 0.0 : (double) value); break; } case BOOLEAN: { - writer.writeBoolean((boolean) value); + writer.writeBoolean(value != null && (boolean) value); break; } default: { diff --git a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/TypedValueImpl.java b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/TypedValueImpl.java index a70a1c9f6..b5114f862 100644 --- a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/TypedValueImpl.java +++ b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/TypedValueImpl.java @@ -33,6 +33,7 @@ */ package org.openjdk.jmc.flightrecorder.writer; +import org.openjdk.jmc.flightrecorder.writer.api.Annotation; import org.openjdk.jmc.flightrecorder.writer.api.TypedValueBuilder; import org.openjdk.jmc.flightrecorder.writer.api.TypedValue; import org.openjdk.jmc.flightrecorder.writer.util.NonZeroHashCode; @@ -142,13 +143,68 @@ public List getFieldValues() { for (TypedFieldImpl field : type.getFields()) { TypedFieldValueImpl value = fields.get(field.getName()); if (value == null) { - value = new TypedFieldValueImpl(field, field.getType().nullValue()); + value = new TypedFieldValueImpl(field, getDefaultImplicitFieldValue(field)); } values.add(value); } return values; } + /** + * Gets the default value for a field when not explicitly provided by the user. + *

+ * For event types (jdk.jfr.Event): + *

+ *

+ * Note: JFR timestamps are stored as ticks relative to the chunk start, so the parser will + * convert this absolute tick value to chunk-relative during reading. + *

+ * Tick Frequency Assumption: This implementation assumes a 1:1 tick frequency + * (1 tick = 1 nanosecond) as currently hardcoded in {@code RecordingImpl}. If the tick + * frequency becomes configurable in the future, {@link System#nanoTime()} values will need to + * be converted to ticks using: {@code nanoTime * ticksPerSecond / 1_000_000_000L}. + * + * @param field + * the field to get default value for + * @return the default value for the field + */ + private TypedValueImpl getDefaultImplicitFieldValue(TypedFieldImpl field) { + if (!"jdk.jfr.Event".equals(type.getSupertype())) { + return field.getType().nullValue(); + } + + // Check if field is annotated with @Timestamp (any value means it's chunk-relative) + if (hasTimestampAnnotation(field)) { + // Use current nanoTime as default - will be valid and >= chunk startTicks + // NOTE: Assumes 1:1 tick frequency (1 tick = 1 ns) as per RecordingImpl line 280 + return field.getType().asValue(System.nanoTime()); + } + + // For all other fields, return null value + // Null builtin values are handled properly by Chunk.writeBuiltinType() + return field.getType().nullValue(); + } + + /** + * Checks if a field has the {@code @Timestamp} annotation. + * + * @param field + * the field to check + * @return true if the field is annotated with @Timestamp + */ + private boolean hasTimestampAnnotation(TypedFieldImpl field) { + for (Annotation annotation : field.getAnnotations()) { + if ("jdk.jfr.Timestamp".equals(annotation.getType().getTypeName())) { + return true; + } + } + return false; + } + long getConstantPoolIndex() { return cpIndex; } diff --git a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/Type.java b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/Type.java index bc10ff225..426566645 100644 --- a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/Type.java +++ b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/Type.java @@ -201,6 +201,34 @@ public interface Type extends NamedType { TypedValue asValue(Object value); /** + * Creates a typed null value for this type. + *

+ * Use this method when you need to pass a null value for optional or missing complex-type + * fields. Passing {@code null} directly to + * {@link TypedValueBuilder#putField(String, TypedValue)} causes compilation ambiguity because + * the method is overloaded with multiple parameter types. + *

+ * For primitive types (int, long, String, etc.), you can pass primitive default/null values + * directly. For complex types (Thread, StackTrace, custom types), use this method to create a + * properly typed null value. + *

+ * Example: + * + *

+	 * {
+	 * 	@code
+	 * 	Types types = recording.getTypes();
+	 * 	Type stackTraceType = types.getType(Types.JDK.STACK_TRACE);
+	 * 	Type threadType = types.getType(Types.JDK.THREAD);
+	 *
+	 * 	Type eventType = recording.registerEventType("custom.Event");
+	 * 	recording.writeEvent(eventType.asValue(builder -> {
+	 * 		builder.putField("startTime", System.nanoTime()).putField("stackTrace", stackTraceType.nullValue()) // typed null
+	 * 				.putField("eventThread", threadType.nullValue()); // typed null
+	 * 	}));
+	 * }
+	 * 
+ * * @return a specific {@linkplain TypedValue} instance designated as the {@literal null} value * for this type */ diff --git a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/TypedValueBuilder.java b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/TypedValueBuilder.java index 3c4338826..2937345ed 100644 --- a/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/TypedValueBuilder.java +++ b/core/org.openjdk.jmc.flightrecorder.writer/src/main/java/org/openjdk/jmc/flightrecorder/writer/api/TypedValueBuilder.java @@ -36,7 +36,40 @@ import java.util.Map; import java.util.function.Consumer; -/** A fluent API for lazy initialization of a composite type value */ +/** + * A fluent API for lazy initialization of a composite type value. + *

+ * This builder provides a chainable interface for setting field values in complex types. Use it + * with {@link Type#asValue(java.util.function.Consumer)} to construct typed values. + *

Handling Null Values

+ *

+ * When setting field values, avoid passing {@code null} directly as it causes compilation ambiguity + * due to overloaded methods. Instead: + *

+ *

+ * Example: + * + *

+ * {
+ * 	@code
+ * 	Types types = recording.getTypes();
+ * 	Type threadType = types.getType(Types.JDK.THREAD);
+ *
+ * 	Type eventType = recording.registerEventType("custom.Event", builder -> {
+ * 		builder.addField("message", Types.Builtin.STRING).addField("thread", Types.JDK.THREAD);
+ * 	});
+ *
+ * 	recording.writeEvent(eventType.asValue(builder -> {
+ * 		builder.putField("message", (String) null) // primitive null with cast
+ * 				.putField("thread", threadType.nullValue()); // complex type null
+ * 	}));
+ * }
+ * 
+ */ public interface TypedValueBuilder { Type getType(); diff --git a/core/tests/org.openjdk.jmc.flightrecorder.writer.test/src/main/java/org/openjdk/jmc/flightrecorder/writer/ImplicitEventFieldsTest.java b/core/tests/org.openjdk.jmc.flightrecorder.writer.test/src/main/java/org/openjdk/jmc/flightrecorder/writer/ImplicitEventFieldsTest.java new file mode 100644 index 000000000..d6923c866 --- /dev/null +++ b/core/tests/org.openjdk.jmc.flightrecorder.writer.test/src/main/java/org/openjdk/jmc/flightrecorder/writer/ImplicitEventFieldsTest.java @@ -0,0 +1,299 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2025, Datadog, Inc. All rights reserved. + * + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * The contents of this file are subject to the terms of either the Universal Permissive License + * v 1.0 as shown at http://oss.oracle.com/licenses/upl + * + * or the following license: + * + * Redistribution and use in source and binary forms, with or without modification, are permitted + * provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this list of conditions + * and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, this list of + * conditions and the following disclaimer in the documentation and/or other materials provided with + * the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors may be used to + * endorse or promote products derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY + * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.openjdk.jmc.flightrecorder.writer; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openjdk.jmc.common.item.Attribute; +import org.openjdk.jmc.common.item.IItem; +import org.openjdk.jmc.common.item.IItemCollection; +import org.openjdk.jmc.common.item.IMemberAccessor; +import org.openjdk.jmc.common.unit.IQuantity; +import org.openjdk.jmc.common.unit.UnitLookup; +import org.openjdk.jmc.flightrecorder.JfrAttributes; +import org.openjdk.jmc.flightrecorder.JfrLoaderToolkit; +import org.openjdk.jmc.flightrecorder.writer.api.Recording; +import org.openjdk.jmc.flightrecorder.writer.api.Recordings; +import org.openjdk.jmc.flightrecorder.writer.api.Type; +import org.openjdk.jmc.flightrecorder.writer.api.Types; + +/** + * Tests for implicit event fields (stackTrace, eventThread, startTime) handling. + *

+ * These tests verify that events can be written without explicitly providing values for implicit + * fields, and that the Writer API automatically provides default values for them. + */ +@SuppressWarnings("restriction") +class ImplicitEventFieldsTest { + private Recording recording; + private Path jfrPath; + + @BeforeEach + void setup() throws Exception { + jfrPath = Files.createTempFile("jfr-writer-test-implicit-", ".jfr"); + recording = Recordings.newRecording(jfrPath); + } + + @AfterEach + void teardown() throws Exception { + if (recording != null) { + recording.close(); + } + if (jfrPath != null) { + Files.deleteIfExists(jfrPath); + } + } + + /** + * Tests that an event can be written without implicit fields explicitly set. + *

+ * This test reproduces the issue where field values appear shifted when implicit fields are not + * provided. After the fix, this should pass. + */ + @Test + void eventWithoutImplicitFields() throws Exception { + Type eventType = recording.registerEventType("test.MinimalEvent", builder -> { + builder.addField("customField", Types.Builtin.LONG); + }); + + // Write event WITHOUT setting implicit fields + recording.writeEvent(eventType.asValue(builder -> { + builder.putField("customField", 12345L); + })).close(); + + // Verify recording parses correctly + IItemCollection events = JfrLoaderToolkit.loadEvents(jfrPath.toFile()); + assertTrue(events.hasItems(), "Recording should contain events"); + + events.forEach(itemType -> { + itemType.forEach(item -> { + // Verify implicit fields have defaults + IQuantity startTime = JfrAttributes.START_TIME.getAccessor(itemType.getType()).getMember(item); + assertNotNull(startTime, "startTime should have a default value"); + + // Verify custom field has correct value (not shifted to startTime) + IMemberAccessor accessor = Attribute + .attr("customField", "customField", UnitLookup.RAW_NUMBER).getAccessor(itemType.getType()); + assertNotNull(accessor, "Accessor for customField should not be null"); + Number customFieldValue = accessor.getMember(item); + assertNotNull(customFieldValue, "customField should have a value"); + assertEquals(12345L, customFieldValue.longValue(), + "customField should be 12345, not shifted to startTime"); + }); + }); + } + + /** + * Tests that explicit startTime values are respected and not overridden by the default. + *

+ * Note: JFR stores timestamps as ticks relative to the chunk start, so the parser converts the + * stored tick value to absolute epoch nanoseconds using the chunk header. We verify that the + * timestamp is reasonable (positive epoch time) rather than checking for exact equality. + */ + @Test + void eventWithExplicitStartTime() throws Exception { + long explicitTime = System.nanoTime(); + Type eventType = recording.registerEventType("test.ExplicitTime"); + + recording.writeEvent(eventType.asValue(builder -> { + builder.putField("startTime", explicitTime); + })).close(); + + IItemCollection events = JfrLoaderToolkit.loadEvents(jfrPath.toFile()); + assertTrue(events.hasItems(), "Recording should contain events"); + + events.forEach(itemType -> { + itemType.forEach(item -> { + IQuantity time = JfrAttributes.START_TIME.getAccessor(itemType.getType()).getMember(item); + assertNotNull(time, "startTime should not be null"); + // Verify that the timestamp is reasonable (epoch time in nanoseconds) + // Should be a recent positive timestamp, not negative or zero + long epochNanos = time.longValue(); + assertTrue(epochNanos > 0L, "Timestamp should be positive (epoch nanos)"); + }); + }); + } + + /** + * Tests an event with only implicit fields and no custom fields. + */ + @Test + void eventWithOnlyImplicitFields() throws Exception { + Type eventType = recording.registerEventType("test.ImplicitOnly"); + + recording.writeEvent(eventType.asValue(builder -> { + // Don't set any fields - rely on defaults + })).close(); + + IItemCollection events = JfrLoaderToolkit.loadEvents(jfrPath.toFile()); + assertTrue(events.hasItems(), "Recording should contain events"); + + events.forEach(itemType -> { + itemType.forEach(item -> { + IQuantity startTime = JfrAttributes.START_TIME.getAccessor(itemType.getType()).getMember(item); + assertNotNull(startTime, "startTime should have a default value"); + }); + }); + } + + /** + * Tests that multiple custom fields maintain correct alignment when implicit fields are not + * provided. + */ + @Test + void eventWithMultipleCustomFields() throws Exception { + Type eventType = recording.registerEventType("test.MultiField", builder -> { + builder.addField("field1", Types.Builtin.LONG).addField("field2", Types.Builtin.STRING).addField("field3", + Types.Builtin.INT); + }); + + recording.writeEvent(eventType.asValue(builder -> { + builder.putField("field1", 111L).putField("field2", "test-string").putField("field3", 333); + })).close(); + + IItemCollection events = JfrLoaderToolkit.loadEvents(jfrPath.toFile()); + assertTrue(events.hasItems(), "Recording should contain events"); + + events.forEach(itemType -> { + itemType.forEach(item -> { + // Verify all custom fields have correct values + // field1 is LONG → raw number + IMemberAccessor field1Accessor = Attribute + .attr("field1", "field1", UnitLookup.RAW_NUMBER).getAccessor(itemType.getType()); + assertEquals(111L, field1Accessor.getMember(item).longValue(), "field1 should be 111"); + + IMemberAccessor field2Accessor = Attribute + .attr("field2", "field2", UnitLookup.PLAIN_TEXT).getAccessor(itemType.getType()); + assertEquals("test-string", field2Accessor.getMember(item), "field2 should be 'test-string'"); + + // field3 is INT → linear number + IMemberAccessor field3Accessor = Attribute.attr("field3", "field3", UnitLookup.NUMBER) + .getAccessor(itemType.getType()); + assertEquals(333, field3Accessor.getMember(item).longValue(), "field3 should be 333"); + }); + }); + } + + /** + * Tests that all builtin types receive proper default values when not explicitly set. + *

+ * This test verifies the fix for builtin type field skipping. When builtin fields are not + * explicitly set, they should receive type-appropriate defaults (0 for numbers, false for + * boolean, null for String) instead of being skipped during serialization, which would cause + * field alignment issues. + *

+ * The test includes a final field with an explicit value to verify that field alignment remains + * correct after all the default builtin fields. + */ + @Test + void eventWithAllBuiltinFieldsUnset() throws Exception { + Type eventType = recording.registerEventType("test.AllBuiltins", builder -> { + builder.addField("byteField", Types.Builtin.BYTE).addField("charField", Types.Builtin.CHAR) + .addField("shortField", Types.Builtin.SHORT).addField("intField", Types.Builtin.INT) + .addField("longField", Types.Builtin.LONG).addField("floatField", Types.Builtin.FLOAT) + .addField("doubleField", Types.Builtin.DOUBLE).addField("booleanField", Types.Builtin.BOOLEAN) + .addField("stringField", Types.Builtin.STRING).addField("finalField", Types.Builtin.LONG); + }); + + // Write event WITHOUT setting builtin field values - all should get defaults + // Set finalField to verify field alignment is correct + recording.writeEvent(eventType.asValue(builder -> { + builder.putField("finalField", 99999L); + })).close(); + + // Verify recording parses correctly and contains the event + IItemCollection events = JfrLoaderToolkit.loadEvents(jfrPath.toFile()); + assertTrue(events.hasItems(), "Recording should contain events"); + + events.forEach(itemType -> { + itemType.forEach(item -> { + // Verify all builtin fields have appropriate default values + // BYTE → linear number + IMemberAccessor byteAccessor = Attribute + .attr("byteField", "byteField", UnitLookup.NUMBER).getAccessor(itemType.getType()); + assertEquals(0, byteAccessor.getMember(item).longValue(), "byteField should default to 0"); + + // SHORT → linear number + IMemberAccessor shortAccessor = Attribute + .attr("shortField", "shortField", UnitLookup.NUMBER).getAccessor(itemType.getType()); + assertEquals(0, shortAccessor.getMember(item).longValue(), "shortField should default to 0"); + + // INT → linear number + IMemberAccessor intAccessor = Attribute + .attr("intField", "intField", UnitLookup.NUMBER).getAccessor(itemType.getType()); + assertEquals(0, intAccessor.getMember(item).longValue(), "intField should default to 0"); + + // LONG → raw number + IMemberAccessor longAccessor = Attribute + .attr("longField", "longField", UnitLookup.RAW_NUMBER).getAccessor(itemType.getType()); + assertEquals(0L, longAccessor.getMember(item).longValue(), "longField should default to 0"); + + // FLOAT → linear number + IMemberAccessor floatAccessor = Attribute + .attr("floatField", "floatField", UnitLookup.NUMBER).getAccessor(itemType.getType()); + assertEquals(0.0, floatAccessor.getMember(item).doubleValue(), 0.001, + "floatField should default to 0.0"); + + // DOUBLE → linear number + IMemberAccessor doubleAccessor = Attribute + .attr("doubleField", "doubleField", UnitLookup.NUMBER).getAccessor(itemType.getType()); + assertEquals(0.0, doubleAccessor.getMember(item).doubleValue(), 0.001, + "doubleField should default to 0.0"); + + IMemberAccessor booleanAccessor = Attribute + .attr("booleanField", "booleanField", UnitLookup.FLAG).getAccessor(itemType.getType()); + assertEquals(false, booleanAccessor.getMember(item), "booleanField should default to false"); + + IMemberAccessor stringAccessor = Attribute + .attr("stringField", "stringField", UnitLookup.PLAIN_TEXT).getAccessor(itemType.getType()); + assertEquals(null, stringAccessor.getMember(item), "stringField should default to null"); + + // Verify the explicit field value is read correctly (proves field alignment is correct) + // LONG → raw number + IMemberAccessor finalAccessor = Attribute + .attr("finalField", "finalField", UnitLookup.RAW_NUMBER).getAccessor(itemType.getType()); + assertEquals(99999L, finalAccessor.getMember(item).longValue(), + "finalField should be 99999, confirming correct field alignment after all default builtin fields"); + }); + }); + } +}