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

python configuration prototype #44

Closed
wants to merge 7 commits into from
Closed

Conversation

codeboten
Copy link
Collaborator

Here's the start of the python prototype. Specifically here, I wanted to test out the ergonomics of using the dictionary object parsed from the config file. I've updated a couple of things in the schema/config file as a result which I'm not opposed to changing back but here's the list of changes:

  1. changed propagators to an array of strings instead of objects. propagators are currently identified by a string in the spec. This means in theory users would be limited in what they can configure in a propagator, but that's the state of the world today.
  2. renamed *_provider to the signal name instead. i think this makes understand the configuration a bit more ergonomic since for example we're configuring exporters for the signal, and then applying them to a specific processor. the user may not need to know this is configuring the provider specifically.
  3. changed processors to the same format as the exporter with type/identifier naming

The code as is supports configuring a console exporter, and turning on/off the SDK. I will continue adding more interpretation of the configuration in the future. The only interface users would interact with is shown in prototype.py as the following:

    otel.configure(otel.parse_and_validate_from_config_file(sys.argv[1], sys.argv[2]))

@@ -0,0 +1,119 @@

import importlib
Copy link

Choose a reason for hiding this comment

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

Nit, use from x import y

@martinkuba
Copy link
Contributor

Question about the processor names - what is the reason that the name includes the exporter name after the slash? I can see it as a more descriptive name, but the type of the processor will have to be parsed from the name.

@codeboten
Copy link
Collaborator Author

Question about the processor names - what is the reason that the name includes the exporter name after the slash? I can see it as a more descriptive name, but the type of the processor will have to be parsed from the name.

It's probably not necessary, since the name is really identifying the type of processor this is and not used as a unique identifier anywhere else. I suppose it could make returning an error when configuring a specific processor a bit more easy to identify, but it's probably not needed at this time

@codeboten codeboten force-pushed the codeboten/py-prototype branch from c67d0fe to e45b943 Compare February 15, 2023 04:21
Alex Boten added 7 commits February 21, 2023 14:39
Signed-off-by: Alex Boten <aboten@lightstep.com>
Updated the list of propagators to an array of strings. Renamed *_provider to the name of the signal instead. This makes searching the parsed configuration a bit cleaner, since exporters are configured per signal, and not necessarily per provider (at least conceptually i found it easier to grok).

Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
Signed-off-by: Alex Boten <aboten@lightstep.com>
@codeboten codeboten force-pushed the codeboten/py-prototype branch from 159b1b5 to bacb1d4 Compare February 21, 2023 23:38

import logging
from typing import List, Sequence, Tuple
from pkg_resources import iter_entry_points
Copy link

Choose a reason for hiding this comment

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

pkg_resources has been deprecated (see this, and also this).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good to know. It's probably ok for the prototype but wouldn't want to ship this code :D

def _resource(self):
# resource detection
# attributes
from opentelemetry.sdk.resources import Resource
Copy link

Choose a reason for hiding this comment

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

Why import here?

Copy link
Collaborator Author

@codeboten codeboten Feb 23, 2023

Choose a reason for hiding this comment

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

not necessary to have the import in the method, that was just me being lazy

Copy link

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just some comments

@codeboten
Copy link
Collaborator Author

Closing in favour of open-telemetry/opentelemetry-configuration#54

@codeboten codeboten closed this Jan 12, 2024
@codeboten codeboten deleted the codeboten/py-prototype branch January 12, 2024 16:12
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.

3 participants