-
Notifications
You must be signed in to change notification settings - Fork 54
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
fix: move manual code to logging_v2 #83
Conversation
@@ -42,4 +53,6 @@ class MetricsServiceV2Client(metrics_service_v2_client.MetricsServiceV2Client): | |||
"LoggingServiceV2Client", | |||
"ConfigServiceV2Client", | |||
"MetricsServiceV2Client", | |||
"ASCENDING", | |||
"DESCENDING", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't logging
meant to be an alias of logging_v2
? Right now it looks like they expose different modules. I can't do logging.logger
, but I can do logging_v2.logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I messed this up.
I think where I'd like to go with this is what Firestore has:
google/cloud/firestore.py
(shim)
google/cloud/firestore_v1/__init__.py
The class Client
(not the module client
) is placed in __all__
, so you can import it under logging_v2
/logging
.
Does that sound alright to you?
from google.cloud import logging
client = logging.Client()
metric = logging.Metric()
This is different from what I have at the current commit (exporting modules) which I don't think makes sense for anything but handlers
.
from google.cloud import logging
client = logging.client.Client()
metric = logging.metric.Metric()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that first one looks good!
What do you think we should do with the auto-generated API (currently in logging_v2
)? I want to discourage users from using the auto-generated modules and use it mostly internally, but there may be some gaps in the hand-written code for now, and it may make sense to allow users to "break the glass". Maybe export them as logging.gapic.MetricsServiceV2Client
? What do you think?
Also, an issue with from google.cloud import logging
is that it collides with the logging
standard library, and they are often going to be used together. Right now we tell users to import as import google.cloud.logging
and use the full path in their code. I wonder if it would make sense to rename the library to google.cloud.cloudlogging
or something so there's no naming conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think we should do with the auto-generated API (currently in logging_v2)? I want to discourage users from using the auto-generated modules and use it mostly internally, but there may be some gaps in the hand-written code for now, and it may make sense to allow users to "break the glass". Maybe export them as logging.gapic.MetricsServiceV2Client? What do you think?
👍 I can remove the generated clients from __all__
and make them available through logging_v2.gapic.MetricServiceV2Client
. This is similar to what you have to do to get to the transport layer in microgenerated clients.
>>> from google.cloud import automl
>>> from google.cloud import automl_v1beta1
>>> automl_v1beta1.services.prediction_service.transports.PredictionServiceTransport
<class 'google.cloud.automl_v1beta1.services.prediction_service.transports.base.PredictionServiceTransport'>
>>> automl.services.prediction_service.transports.PredictionServiceTransport
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: module 'google.cloud.automl' has no attribute 'services'
Also, an issue with from google.cloud import logging is that it collides with the logging standard library, and they are often going to be used together. Right now we tell users to import as import google.cloud.logging and use the full path in their code. I wonder if it would make sense to rename the library to google.cloud.cloudlogging or something so there's no naming conflict
Ah I didn't realize that was a problem. This is definitely a good time to do any naming changes. The generator is flexible on namespacing, so it is really up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove the generated clients from
__all__
and make them available throughlogging_v2.gapic.MetricServiceV2Client
. This is similar to what you have to do to get to the transport layer in microgenerated clients.
Ok, that would be great!
Ah I didn't realize that was a problem. This is definitely a good time to do any naming changes. The generator is flexible on namespacing, so it is really up to you.
Hmm ok, I'll discuss this with the team and get back to you. Let me know if you have strong opinions one way or the other, because I can definitely see pros and cons to each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've removed the 3 generated clients from logging_v2
/logging
. PTAL. Also opened #88 to track a potential naming change.
from google.cloud import logging_v2
logging_v2.gapic.logging_service_v2_client.LoggingServiceV2Client()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good for the most part, but I noticed that some modules are still only accessible through logging_v2
rather than logging
. Like google.cloud.logging_v2.sink
and google.cloud.logging_v2.metric
. Shouldn't the alias be the same for both?
Edit: actually, it looks like the important classes are already exported from each of those files. I think it looks good then!
In preparation for moving to the microgenerator, move all manual code into
logging_v2
and havelogging
serve as an alias.Note: This is targeting the
microgen
branch, notmaster
. I'll continue to make PRs tomicrogen
to make the reviews more manageable.