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

feat: Add json setting to allow unicodes to show in log instead of ascii ch… #193

Merged
merged 3 commits into from Mar 15, 2021

Conversation

ysde
Copy link
Contributor

@ysde ysde commented Feb 23, 2021

…aracter (for example, show chinese word '哈' in log instead of '\u8bed')

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<192> 🦕

@ysde ysde requested review from as code owners Feb 23, 2021
@product-auto-label product-auto-label bot added the api: logging label Feb 23, 2021
@google-cla google-cla bot added the cla: yes label Feb 23, 2021
To allow unicodes to show in log instead of ascii character (for example, show chinese word '哈' in log instead of '\u8bed')
@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Feb 23, 2021

Thanks for the contribution. I agree with the root problem (Unicode characters should be kept in the final logs), but I'm not sure if this is the best way to go about it:

  • We should try to fix this in a way that applies to all environments, instead of just the Container Engine handler.
  • It seems like unicode should be the default behavior, instead of an option that needs to be turned on.
  • Before merging this, we'd want to make sure we put a bunch of tests in place (environment, system, and unit).

If you're interested in making those changes to this PR, that would be great. Otherwise, I can open a new issue and make sure it's addressed in a future release (this seems like it would be high priority on my priorities, so likely next release. But I'll have to do some more testing to make sure)

@ysde
Copy link
Contributor Author

@ysde ysde commented Feb 24, 2021

Hi @daniel-sanche

Sure I'd like to make those changes to this PR. Do you have any suggestions where or which files should I started it?
And what should I do to pass all the checks ?

Thank you very much

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Feb 26, 2021

The first step would be to add a new test to the new environment test submodule to see which GCP platforms are impacted. I did that part myself here (you can take over the submodule, branch, or copy/paste the code)

It looks like only GKE is having issues there, which makes it simple (possibly cloud run too, but I think that's flakiness in the new tests - I'm looking into it). You can probably keep the json.dumps(payload, ensure_ascii=False) part of your code to fix it, but I don't think we need the other parts (I can't think of any situations where it would be useful to turn it off, unless you disagree)

After you have those fixes in and the environment test is passing, you should add similar checks to the unit and system tests.

Our test system is in the process of being upgraded, so it may be a bit confusing/flaky. Feel free to reach out or re-assign the work back to me if you get stuck with the tests!

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Mar 5, 2021

Hey, any updates? If you're finding it hard to navigate the tests, I'm happy to take this over to get it in the next release. Let me know

@ysde
Copy link
Contributor Author

@ysde ysde commented Mar 6, 2021

Hi @daniel-sanche

Sorry for the late response, busy at working recently. :(

I'm not sure if there is some circumstances need to set ensure_ascii=True, so I was trying to keep the same behavior of the program, and think maybe add a json_ensure_ascii setting, default is true is good.

I have little experience of using submodule and not sure how to take over environment test submodule.

I've modified test_container_engine.py, but have ModuleNotFoundError: No module named 'google.cloud' when I try to execute pytest -s test_container_engine.py

Can you give me this weekend to figure out how to finish it?

If it's releasing soon, you can take it over, no worries. : )

Thank you for the help and reviews.

@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Mar 6, 2021

Ok, to make things simple you can ignore the environment test submodule, I can handle adding a test there after you're done.

For the unit tests, you should be able to run them through nox, which will simplify dependency issues. Information can be found in the repo's CONTRIBUTING.md file.

Let me know if you have other questions!

@ysde ysde changed the title Add json setting to allow unicodes to show in log instead of ascii ch… feat: Add json setting to allow unicodes to show in log instead of ascii ch… Mar 7, 2021
@ysde
Copy link
Contributor Author

@ysde ysde commented Mar 7, 2021

@daniel-sanche

Thanks! nox is so cool! I passed nox -s --unit-3.6

And how to make the required checks pass?

Kokoro
Samples - Lint
Samples - Python 3.6
Samples - Python 3.7
Samples - Python 3.8

Thank you

@parthea parthea added the kokoro:force-run label Mar 7, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Mar 7, 2021
@daniel-sanche
Copy link
Contributor

@daniel-sanche daniel-sanche commented Mar 8, 2021

And how to make the required checks pass?

It looks like the CI tests are now passing. Are you still having issues?

@ysde
Copy link
Contributor Author

@ysde ysde commented Mar 9, 2021

And how to make the required checks pass?

It looks like the CI tests are now passing. Are you still having issues?

Nope, CI's just not triggered that time, thank you ! :)

@daniel-sanche daniel-sanche merged commit e8c8e30 into googleapis:master Mar 15, 2021
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants