feat (processor/k8sattributes): wait for synced when starting k8sattributes processor.

[h0cheung]

Description:

When starting k8sattributes processor, block until an initial synchronization has been completed. This solves #32556

Link to tracking Issue:

fix #32556

Testing:

Tested in a cluster with constant high span traffic, no more spans with unassociated k8s metadata after adding this pr.

Documentation:

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[orloffv]

@h0cheung Hi there, I was wondering if you have the time to finalize and merge the pull request. Looking forward to your update. Thanks!

[fatsheep9146]

Thanks for your patience!

I have an advice about the two new options start_timeout and error_when_timeout.

The reason for this enhancement is to make sure the k8sattributes processor will start consuming the incoming traces or metrics until the initial synchronization are completed,
otherwise, some traces/metrics/logs may be unassociated with k8s metadata.

But I think some users may care more about the start rate of collector than the completeness of the k8s metadata.

So how about we change this two fields to

• force_wait_for_cache_synced bool default is false
• wait_for_synced_timeout time.Duration

The users who care more about the completeness could set force_wait_for_cache_synced bool to true and if the synced is still not completed after timeout wait_for_synced_timeout, the collector should directly return error instead of running, since the completeness could not be guaranteed.

@h0cheung @TylerHelmuth

[h0cheung]

Thanks for your patience!

I have an advice about the two new options start_timeout and error_when_timeout.

The reason for this enhancement is to make sure the k8sattributes processor will start consuming the incoming traces or metrics until the initial synchronization are completed, otherwise, some traces/metrics/logs may be unassociated with k8s metadata.

But I think some users may care more about the start rate of collector than the completeness of the k8s metadata.

So how about we change this two fields to

• force_wait_for_cache_synced bool default is false
• wait_for_synced_timeout time.Duration

The users who care more about the completeness could set force_wait_for_cache_synced bool to true and if the synced is still not completed after timeout wait_for_synced_timeout, the collector should directly return error instead of running, since the completeness could not be guaranteed.

@h0cheung @TylerHelmuth

I agree. What are your thoughts? @TylerHelmuth

[orloffv]

@TylerHelmuth can you see, plz?

[TylerHelmuth]

@fatsheep9146 I like that idea. I suggest adjusting naming it wait_for_metadata and wait_for_metadata_timeout to hide some of the details from the end user. I believe we use the term metadata already to refer to the pod data that is gathered and held.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[h0cheung]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

remove stale

[h0cheung]

@fatsheep9146 I like that idea. I suggest adjusting naming it wait_for_metadata and wait_for_metadata_timeout to hide some of the details from the end user. I believe we use the term metadata already to refer to the pod data that is gathered and held.

Thanks. These two configuration items look simple and clear. I think we can use them.

[valner]

@h0cheung Hi, could you take some time to work out the code and merge the pull request? I’d really appreciate your update on this. Thanks a lot

[ChrsMark]

One minor wording improvement :).

[ChrsMark]

@fatsheep9146 @dmitryax could you take a look as well? Otherwise this should be good to go.

[TylerHelmuth]

@fatsheep9146 @dmitryax I plan to merge by end of week, please comment if you have objections

[fatsheep9146]

LGTM! Thanks