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

[exporter/elasticsearch] remove dedup config #33776

Merged
merged 12 commits into from
Jul 16, 2024

Conversation

axw
Copy link
Contributor

@axw axw commented Jun 26, 2024

Description:

Remove the dedup configuration setting, and always de-duplicate. Elasticsearch does not permit duplicate keys in JSON objects, and this configuration is adding more complexity to the code than it's worth.

I've simplified the internal/objmodel API slightly, unexporting the Sort methods, which are internally called by the now unconditional call to Dedup.

Link to tracking Issue:

Closes #33773

Testing:

Ran the unit tests, which cover deduplication. None of the tests in package elasticsearchexporter covered dedup: false.

Documentation:

@andrzej-stencel
Copy link
Member

@axw Can you split the dedot change into a separate PR please?

@axw
Copy link
Contributor Author

axw commented Jun 26, 2024

@andrzej-stencel I've opened #33779 and #33780 for just the deprecations. I'll mark this one as draft and reopen it after those deprecations are released.

@axw axw marked this pull request as draft June 26, 2024 13:02
@axw axw force-pushed the elasticsearch-always-dedup branch 4 times, most recently from b9e0ec2 to 2483f30 Compare July 2, 2024 03:48
@github-actions github-actions bot added the cmd/otelcontribcol otelcontribcol command label Jul 2, 2024
Remove the dedup configuration setting, and always deduplicate.
Elasticsearch does not permit duplicate keys in JSON objects,
and this configuration is adding more complexity to the code
than it's worth.
@axw axw force-pushed the elasticsearch-always-dedup branch from 2483f30 to f8449c8 Compare July 2, 2024 05:36
@axw axw marked this pull request as ready for review July 2, 2024 06:20
@axw axw requested a review from andrzej-stencel July 2, 2024 06:22
Copy link
Contributor

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

one nit otherwise lgtm

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

According to the docs:

the option (...) MAY be removed after N+2 or 6 months, whichever comes later

I believe this means we should not remove this option before January 2025 (six months after v0.104.0). We can hide the option behind a feature flag if it helps (not sure if it makes sense in this case?).

You can still make the feature non-operational according to this:

the option MAY be made non operational already by the same version where it is deprecated

axw added 2 commits July 9, 2024 13:48
It's against policy to remove within 6 months
of deprecation, but it can be made a no-op.
@axw
Copy link
Contributor Author

axw commented Jul 9, 2024

@andrzej-stencel thanks for pointing out the docs. I've reinstated the config, but made it a no-op. I didn't add it back into the README though - let me know if you disagree with that.

the option and the WARN level message MAY be removed after N+2 or 6 months, whichever comes later

I find the "MAY" in this to be a little bit ambiguous. Perhaps it would be worth rewording to:

"the option and WARN level message MUST NOT be removed earlier than N+2 o 6 months, whichever comes later"

WDYT?

@axw axw requested a review from andrzej-stencel July 9, 2024 06:53
@andrzej-stencel
Copy link
Member

I didn't add it back into the README though - let me know if you disagree with that.

I think if the option exists, it should be described in the README - saying it is deprecated and non-functional.

I find the "MAY" in this to be a little bit ambiguous. Perhaps it would be worth rewording to:
"the option and WARN level message MUST NOT be removed earlier than N+2 o 6 months, whichever comes later"

I agree that the wording might be a bit misleading. Your proposal seems OK, please send a PR to update the wording, let's see what other maintainers think.

@andrzej-stencel andrzej-stencel merged commit e495816 into open-telemetry:main Jul 16, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 16, 2024
@axw axw deleted the elasticsearch-always-dedup branch July 16, 2024 09:22
mx-psi pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Aug 16, 2024
#### Description

Clarify that deprecated config options are expected to remain for at
least 6 months after deprecation.

"MUST NOT" makes it clear for readers that this is something that
definitely should not be done, vs. something that's discretionary.

See also
open-telemetry/opentelemetry-collector-contrib#33776 (comment)
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.

[exporter/elasticsearch] deprecate/remove dedup config
5 participants