Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial update to have attributes use the ordered map so the order is maintained into clickhouse #34598

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JustinMason
Copy link

@JustinMason JustinMason commented Aug 11, 2024

Description: Update to maintain attribute order for records created in ClickHouse

Otel Records where not consistently maintain order. Go does not maintain order for maps. This was causing records to be duplicated since ClickHouse considers different attribute order as unique records. This was also effecting compression because these records were considered different if the order was different.

Link to tracking Issue: #33634

Implemented the column.MapIterator and changed the use of attributes from map[string]string to column.MapIterator
ClickHouse/clickhouse-go#1152

Testing:
Updated Unit tests and added coverage for OrderMap type.

Basic integration tested was manually done using ClickHouse Server in a local environment.

Documentation:

Pre-existing use of the attribute columns were switched out to use the MapIterator. The Exporter explicitly sorts the attributes before inserting. The MapIterator maintains the original order so it is not lost on record creation.

@SpencerTorres
Copy link
Member

Awesome to see this. Will review thoroughly once marked as ready, but so far things look good. Especially interested in seeing this for log/trace attributes.

@JustinMason JustinMason force-pushed the feature/clickhouse-exporter-ordered-map-for-attributes branch from 6eca859 to 1277292 Compare August 13, 2024 20:09
@JustinMason JustinMason force-pushed the feature/clickhouse-exporter-ordered-map-for-attributes branch from 1277292 to 60c729b Compare August 13, 2024 20:23
@JustinMason JustinMason marked this pull request as ready for review August 13, 2024 20:26
@JustinMason JustinMason requested a review from a team August 13, 2024 20:26
Copy link
Member

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

Excellent! Glad to see this applies to logs/traces now. Can you confirm the trace test file doesn't require any changes?

Copy link
Contributor

@Frapschen Frapschen left a comment

Choose a reason for hiding this comment

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

thanks!, LGTM

@JustinMason
Copy link
Author

Excellent! Glad to see this applies to logs/traces now. Can you confirm the trace test file doesn't require any changes?

I added field mapping coverage for the attribute conversion to OrderedMap.

FYI, I did notice Tracing Scope().Attributes() does not have a column defined in the clickhouse schema, so those values are not persisted. Was this intentional?

For example: Logs has it https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/clickhouseexporter/exporter_logs.go#L159

Tracing does not: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/exporter/clickhouseexporter/exporter_traces.go#L176

I can create an issue for this, it should be independent of this PR.

@SpencerTorres
Copy link
Member

FYI, I did notice Tracing Scope().Attributes() does not have a column defined in the clickhouse schema, so those values are not persisted. Was this intentional?

Not sure, I'll have to check the OTel spec to see what's in there. Let me know your thoughts on adding it and I'll add it in my exisitng PR #34245

@JustinMason
Copy link
Author

FYI, I did notice Tracing Scope().Attributes() does not have a column defined in the clickhouse schema, so those values are not persisted. Was this intentional?

Not sure, I'll have to check the OTel spec to see what's in there. Let me know your thoughts on adding it and I'll add it in my exisitng PR #34245

Looking at the spec it looks like the attributes are optional for tracing, so its probably a good idea to add them in case they are being used in an up stream component.
https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer

[since 1.13.0] attributes (optional): Specifies the instrumentation scope attributes to associate with emitted telemetry.

@earwin
Copy link

earwin commented Sep 11, 2024

I wrote a similar IterableOrderedMap some while ago, and came here to report the same problem to see it's already being taken care of : D Thanks!
I'll leave my code here for inspiration: https://gist.github.com/earwin/149740749c8aaccde5ea626169c32735
It uses a single in-place sorted slice and generics, so mayhaps is easier on GC.
All stuff using iter.Seq is go1.23 sugar and can be dropped.

@hanjm
Copy link
Member

hanjm commented Sep 11, 2024

I wrote a similar IterableOrderedMap some while ago, and came here to report the same problem to see it's already being taken care of : D Thanks!

I'll leave my code here for inspiration: https://gist.github.com/earwin/149740749c8aaccde5ea626169c32735

It uses a single in-place sorted slice and generics, so mayhaps is easier on GC.

All stuff using iter.Seq is go1.23 sugar and can be dropped.

@earwin so good ideal for go 1.23, can you put it to clickhouse go sdk?

@earwin
Copy link

earwin commented Sep 11, 2024

@earwin so good ideal for go 1.23, can you put it to clickhouse go sdk?

I've been meaning to say it's perfectly usable for pre-1.23 as well, because everything that uses iter.Seq is non-core functionality which can be deleted.

I'll ask and see if they are open to having an impl out-of-the-box. ... there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants