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

Propose "Named Tracers" #16

Merged
merged 34 commits into from
Sep 26, 2019

Conversation

z1c0
Copy link
Contributor

@z1c0 z1c0 commented Aug 12, 2019

No description provided.

text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I am requesting more changes because:

  1. This does not address the Metrics part;
  2. This change adds the notion of a Resource back in the API. I do agree that the Resource is a good concept to have, but I think this belongs to the implementation (in our case SDK) not the API.

text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
@z1c0
Copy link
Contributor Author

z1c0 commented Sep 25, 2019

@bogdandrutu I'm not quite sure how we left off after yesterday's discussion in the spec meeting. Are you OK with the approach that we propose a dedicated RFC for metrics (and also logs at some later point in time) to have a better separation of concerns?
Metrics and logs get a mention in "Future possibilities" (i.e. "very near future" for metrics).

@bogdandrutu
Copy link
Member

@z1c0 I would prefer to not postpone the metrics:

  1. This is a major API change that we need for Meter as well before alpha.
  2. All reasons that we need this for Trace apply to Metrics as well.
  3. We want to be consistent between Meter/Tracer in general if possible.

I suggest that we just re-write a bit the first few paragraphs to generalize the motivation for metrics as well.

Everything else stays the same:

// Create a tracer/meter for given instrumentation library in a specific version.
Tracer tracer = OpenTelemetry.getTracerFactory().getTracer("io.opentelemetry.contrib.mongodb", "semver:1.0.0");
Meter meter = OpenTelemetry.getMeterFactory().getMeter("io.opentelemetry.contrib.mongodb", "semver:1.0.0");

Also do we need a way to access unnamed Tracer/Meter?

@z1c0
Copy link
Contributor Author

z1c0 commented Sep 26, 2019

@bogdandrutu metrics are now included in this proposal.
Unnamed Tracers/Meters are mentioned in the last "Explanation" paragraph.

text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
text/0000-named-tracers.md Outdated Show resolved Hide resolved
z1c0 and others added 3 commits September 26, 2019 13:34
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM, subject to addressing Bogdan's comments regarding having uniform handling for Metrics as well (or a plan for it if it is not addressed in this PR).

@Oberon00
Copy link
Member

Seems like this is ready to merge then?

@bogdandrutu bogdandrutu merged commit ea929a7 into open-telemetry:master Sep 26, 2019
@bogdandrutu
Copy link
Member

@z1c0 please in parallel start two PRs:

  1. To mark this approved.
  2. A PR to change specs accordingly.

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

Successfully merging this pull request may close these issues.