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

Rename BatchTimeout to ScheduleDelay #2564

Closed
wants to merge 2 commits into from

Conversation

sincejune
Copy link
Contributor

@codecov
Copy link

codecov bot commented Jan 29, 2022

Codecov Report

Merging #2564 (b4b468f) into main (3cf35bd) will decrease coverage by 0.0%.
The diff coverage is 83.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2564     +/-   ##
=======================================
- Coverage   76.0%   76.0%   -0.1%     
=======================================
  Files        174     174             
  Lines      12091   12091             
=======================================
- Hits        9194    9190      -4     
- Misses      2652    2656      +4     
  Partials     245     245             
Impacted Files Coverage Δ
.../otlp/otlptrace/internal/otlptracetest/otlptest.go 0.0% <0.0%> (ø)
sdk/trace/batch_span_processor.go 80.2% <100.0%> (-1.9%) ⬇️

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

This would break a stable API and cannot be accepted. An additional option could be added that is synonymous, but we cannot remove the existing option.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 30, 2022

This would break a stable API and cannot be accepted. An additional option could be added that is synonymous, but we cannot remove the existing option.

Agreed. I would not even recommend adding an additional option as the specification makes no statements requiring or recommending variable or options be named with a particular value. It does specify the environment variable name we need to support, but that is not the same as specifying a name for internal code.

It would be nice to have our names match the specification, but given our stable status and the added confusion of having two options for the same function I am not in favor of accepting changes to the name.

@Aneurysm9 Aneurysm9 added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package wontfix This will not be worked on labels Jan 30, 2022
@Aneurysm9 Aneurysm9 closed this Jan 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants