Log the full TableMetadata

[andrew4699]

Description

Adds more logs. This is useful for determining the types of workloads happening on Polaris.

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

• Added log assertions

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

[eric-maynard]

This can be pretty big to log into INFO

[andrew4699]

Yea I'm both worried about the performance hit of serializing such a large object and the amount of data logged. Some possible options:

• Move to DEBUG (still performs serialization, does nothing for people who run in INFO+ which should be common)
• Gate the entire thing behind a featureConfiguration
• Set a limit on the log output length in featureConfiguration (still performs serialization)

What are your thoughts? I'm leaning toward the 2nd one.

[eric-maynard]

Move to DEBUG (still performs serialization, does nothing for people who run in INFO+ which should be common)

Good callout. Man do I miss scala.

In this case, perhaps we can do something like:

      LOGGER.info(
          "doCommit full new metadata: {}",
          () -> PolarisObjectMapperUtil.serialize(getCurrentPolarisContext(), metadata));

SLF4J 2.0 supports this, and in general it might be a useful wrapper for us.

Having said all of that, I'm less worried about the serde overhead, since we do so much metadata/property serde anyway, and more about emitting a huge blob into the logs. Particularly the INFO logs which should stay high-signal.

[andrew4699]

Oh that's nice! I updated it to DEBUG and made it use the supplier syntax.

[andrew4699]

I removed the ReportMetricsRequest logging as it seems like we already log it as a tag with Invoking CatalogApi with params.

[snazy]

Thanks for the PR @andrew4699.

As mentioned in the other comment, this can be a really huge logged string in the range of multiple megabytes of JSON. I'm not sure whether that's a good idea. My concerns around this are:

• Increased cost when using a logging SaaS
• Huge log files
• Exposing user data in log files

I'd be much in favor of not merging this change as unconditionally, even at debug level, logging will produce a huge amount log data.

Debug logging is often used to investigate service issues - but too excessive debug logging is often not really helpful.

[RussellSpitzer]

I would consider this probably a "trace" level event. I agree with @snazy's comments here. I would hesitate to log these whole entities as well for security reasons.

[andrew4699]

@RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 5 Snapshot ID & summaries?

[snazy]

@RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 5 Snapshot ID & summaries?

What's the use case for logging it at all, that cannot be done from an Iceberg client or curl?

[RussellSpitzer]

@RussellSpitzer @snazy Thank you for the feedback. I'm new to the Iceberg space and appreciate the context you have on these objects. My hope is to make it easier to see the "unstructured" data that gets self-reported by the query engines. Would you feel more comfortable if this was scoped down to printing the last 5 Snapshot ID & summaries?

What's the use case for logging it at all, that cannot be done from an Iceberg client or curl?

I have no problem with the functionality but I think it probably should be part of an eventing api. We may want to keep the complete history of the table somewhere (possibly not in the Catalog itself)