Skip to content

Conversation

jonatan-ivanov
Copy link
Member

@jonatan-ivanov jonatan-ivanov commented Jul 17, 2025

Similarly to #4994 this PR adds a fallback value to all the KeyValues used by DefaultMongoHandlerObservationConvention.

TODO:

  • Read the Spring Data contribution guidelines
  • Use the code formatters provided here
  • Add test cases
  • Added myself as author in the headers of the classes I touched
  • Maybe rebasing it on top of 4.4.x

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 17, 2025
* @return key value
*/
public KeyValue withOptionalValue(@Nullable String value) {
return withValue(ObjectUtils.isEmpty(value) ? KeyValue.NONE_VALUE : value);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if "UNKNOWN" is a better default value for most of these rather than none.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to have a bit more encapsulated API for a more declarative approach to reduce the likelihood for using the API wrong. Something along the lines of:

// TODO: it would be nice to have a getKeyName() method returning KeyName from KeyValue for usage in e.g. KeyName[] getLowCardinalityKeyNames() 
static KeyValue DB_SYSTEM = KeyValue.of("db.system", "mongodb");


static SmartKeyName<ConnectionString> DB_CONNECTION_STRING = SmartKeyName.required("db.connection_string",
		ConnectionString::getConnectionString);

later:

@Nullable ConnectionString connectionString = context.getConnectionString();

KeyValues keyValues = KeyValues.of(MongoObservation.DB_SYSTEM,
		MongoObservation.DB_CONNECTION_STRING.valueOf(connectionString));

You can find SmartKeyName (needs probably a better name) at
https://gist.github.com/mp911de/6729ca327326d072820a1a0eb225c4a6

Copy link
Member Author

Choose a reason for hiding this comment

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

@shakuzen

I wonder if "UNKNOWN" is a better default value for most of these rather than none.

I think so, I can also make the fallback mandatory so it needs to be decided for every key-value.

@mp911de

I would like to have a bit more encapsulated API for a more declarative approach [...]

We can consider improving on this in Micrometer 1.16 (let me talk to @shakuzen about it) but I think this should be considered as a bug and should be fixed in earlier versions where Micrometer 1.16 will not be available I think (unless you want to copy some code from there).

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should make this easier to get right (and harder to get wrong). Let's see what we can come up with. Mark's SmartKeyName looks like a good start. @jonatan-ivanov can you make an issue in Micrometer so we track the work?

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}

return keyValues;
return KeyValues.of(
DB_SYSTEM.withValue("mongodb"),
Copy link
Member

Choose a reason for hiding this comment

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

I see your approach and I like it more than concatenating KeyValues. With a refined approach regarding value extraction, we might get a chance for improvement.

@mp911de mp911de self-assigned this Aug 4, 2025
@mp911de mp911de added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 4, 2025
@mp911de mp911de marked this pull request as ready for review August 7, 2025 08:45
@mp911de mp911de force-pushed the observation-keyvalues-fallback branch from a9341f3 to 2b62a12 Compare August 7, 2025 10:42
@mp911de
Copy link
Member

mp911de commented Aug 7, 2025

I pushed a refined variant of a contextual observer that simplifies usage a bit.

christophstrobl pushed a commit that referenced this pull request Aug 14, 2025
christophstrobl pushed a commit that referenced this pull request Aug 14, 2025
christophstrobl pushed a commit that referenced this pull request Aug 14, 2025
christophstrobl pushed a commit that referenced this pull request Aug 14, 2025
christophstrobl pushed a commit that referenced this pull request Aug 14, 2025
christophstrobl added a commit that referenced this pull request Aug 14, 2025
Update javadoc and remove deprecated observation keys.

Original Pull Request: #5020
@christophstrobl christophstrobl added this to the 4.4.9 (2024.1.9) milestone Aug 14, 2025
@christophstrobl
Copy link
Member

Thank you @jonatan-ivanov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants