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

Update Python agent-config #2272

Merged
merged 8 commits into from
Mar 9, 2023
Merged

Update Python agent-config #2272

merged 8 commits into from
Mar 9, 2023

Conversation

ronyis
Copy link
Contributor

@ronyis ronyis commented Feb 5, 2023

* Removing usage of `otlp_proto_grpc/http`
* General fixes
@ronyis ronyis requested review from a team as code owners February 5, 2023 16:02
Comment on lines 38 to 40
metrics are being exported to `console` (stdout). It is currently required for
your to specify a metrics exporter. If you aren't exporting metrics, specify
`none` as the value instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's still required to set a metrics exporter (probably not). Could also remove this part.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least as of last December it was needed (in my own testing). Otherwise you'd get an error at runtime. I'd love it if this isn't required anymore, though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for late reply -- I just confirmed that this is still necessary with the latest agent.

@svrnm
Copy link
Member

svrnm commented Feb 6, 2023

@open-telemetry/python-approvers , @open-telemetry/python-maintainers: PTAL

ronyis and others added 2 commits February 6, 2023 18:43
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@cartermp
Copy link
Contributor

cartermp commented Mar 8, 2023

/easycla

@cartermp
Copy link
Contributor

cartermp commented Mar 8, 2023

@ronyis since you submitted this change, we've applied prettier-style formatting to the repo and added a check that's now failing. Would you mind running npm run pretter:write on this file? Might be easier to first rebase.

@ronyis
Copy link
Contributor Author

ronyis commented Mar 9, 2023

Sure, done @cartermp

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

LGTM
(FYI, I'll make a pass after this lands.)

@cartermp cartermp merged commit 89e594c into open-telemetry:main Mar 9, 2023
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.

None yet

4 participants