why do we need to check if c.Stats is nil here? Shouldn't it be initialized previously?

I believe this checks for the case where old stats is nil here

Also get a memory panic if I take it out

Hmm, where do we actually assign c.Stats? Is it ever assigned?

As I see we are only returning *containerstorestats.ContainerStats in generatedContainerMetrics, but we are not actually storing the stats on the container object? Is that right? If that's the case, then c.Stats will always be nil...

Yep, good point -- actual caching of the metrics should be fixed now.

What's the rationale for struct embedding here?

All stats whose additional metrics will eventually be returned via Prometheus format. Rationale would be to add the additional caching logic in the future, and since stats and metrics will be set in the same place (reading the same linux cgroup file) we can read from the container stats store or sandbox stats store.

Maybe my question wasn't clear. What's the reason for embedding versus using fields?

// embedding
type ContainerStats struct {
	ContainerCPUStats
	ContainerMemoryStats
	ContainerFileSystemStats
}

// fields
type ContainerStats struct {
	CPU        ContainerCPUStats
	Memory     ContainerMemoryStats
	FileSystem ContainerFileSystemStats
}

Each of these three structs (ContainerCPUStats, ContainerMemoryStats, and ContainerFileSystemStats contain a Timestamp field. Since all three are then embedded into ContainerStats, only one of the three Timestamp fields will be visible as ContainerStats.Timestamp; the other two will be shadowed. This can be a source of confusing behavior; if we do need to have this embedding pattern, please move Timestamp out of the individual structs and up into ContainerStats directly.

What's the sample window?

Existing code using the ContainerStats.Timestamp field are seeing a time.Time. This code changes it to an int64. Is there a specific reason why? Generally time.Time would be preferred as a standard type for representing time.

CRI fields use int64, figured it would be easier than converting

Since this is an internal struct that needs to be converted on the way out anyway, I think it'd make more sense to centralize the conversion and maintain the standard time.Time here.

I'm not sure I can parse "when it was accessed". Does this mean you're tracking the total number of bytes over the lifetime of a container (so maybe you mean the max usage?).

This function appears to do nothing if there is already a stats.ContainerStats attached to the container or sandbox. Is that intentional?

It's odd to store a pointer to a struct, and then continue to mutate the struct (lines 115 and 121). It looks like this breaks the intent of the mutex in the sandbox.Store and container.Store, opening up the possibility of a data race.

It doesn't look like setContainerStats is ever called with isSandbox set to true. Can you remove the parameter and the codepath?

The ID and isSandbox parameters appear unused. Can you remove them?

The ID parameter appears unused. Can you remove it?