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

Consolidated OTel ES IndexNameProviders #2458

Merged

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Sep 5, 2020

Which problem is this PR solving?

There were two indexNameProviders with very similar code. One for reading and one for writing. These have been consolidated into one provider in the internal/esutil package

Short description of the changes

  • Consolidated IndexNameProviders and tests

Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott requested a review from a team as a code owner September 5, 2020 01:22
@joe-elliott joe-elliott requested a review from vprithvi September 5, 2020 01:22
@joe-elliott joe-elliott changed the title Consolidated index name providers Consolidated OTel ES IndexNameProviders Sep 5, 2020
@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #2458 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2458      +/-   ##
==========================================
+ Coverage   95.57%   95.59%   +0.01%     
==========================================
  Files         208      208              
  Lines       10690    10690              
==========================================
+ Hits        10217    10219       +2     
+ Misses        401      399       -2     
  Partials       72       72              
Impacted Files Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5508165...7c50082. Read the comment docs.

Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

@pavolloffay If you have a chance to look at this I would appreciate it.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

It looks good but the package should be changed. I am fine to keep it in reader or exporter packages or move it to internal under something else than esclient.

@@ -12,39 +12,54 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package esspanreader
package esclient
Copy link
Member

Choose a reason for hiding this comment

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

It should go to a different package than esclient. The esclient is dedicated only for the ES client used by Jaeger writer/reader.

Copy link
Member Author

Choose a reason for hiding this comment

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

moved to ./internal/esutil

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@pavolloffay pavolloffay merged commit 5d0a0fa into jaegertracing:master Sep 11, 2020
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.

2 participants