-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add fonts to support more different types of characters for multiple languages #29861
Conversation
…iple languages [fix elastic#29495]
This pull request does not have a backport label. Could you fix it @lucasfcosta? 🙏
NOTE: |
LGTM, tested all those sites myself, they all work |
Pinging @elastic/uptime (Team:Uptime) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
@@ -69,7 +69,12 @@ RUN apt-get update -y && \ | |||
libcairo2\ | |||
libasound2\ | |||
libatspi2.0-0\ | |||
libxshmfence1 && \ | |||
libxshmfence1 \ | |||
fonts-arphic-ukai \ |
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 am curios about the list we arrived at here, We don't even have the basic one fonts-liberation
for the image.
Do we know if that works?
Here is the list from PW for comparison - https://github.com/microsoft/playwright/blob/5ba7903ba098586a13745e0d7ac894f1d55d47aa/packages/playwright-core/src/utils/nativeDeps.ts#L434-L470
One we use in RUM agent for puppeteer and playwright - https://github.com/elastic/apm-agent-rum-js/tree/master/.ci/docker
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.
Here I followed an approach of testing websites and taking note of which didn't display all characters correctly. Then, I went after the packages only for those, and ran an E2E test which generated the images above. I followed this process to avoid adding too many unnecessary packages.
Wrt to fonts-liberation, it seemed to be a standard set of fonts which could just replace the default set of fonts to which it's compatible, but we already have default fonts for western characters in the base image and thus I thought we wouldn't need that. Looking into the other packages, they also seem to fall into one of two categories:
- Covering standard character sets (like
fonts-liberation
orttf-unifont
) - Tools for managing fonts themselves (like
libfontconfig
)
The other fonts seemed to be the most complete and popular fonts for their character standards (fonts-arphic-ukai
[for chinese characters] , fonts-ipafont-mincho
[for japanese characters, fonts-ipafont-gothic
being for sans-serif japanese characters], and fonts-unfonts-core
for Korean characters). Those were the ones which came up when I looked for packages for each individual language.
We already had support for Arabic/Russian characters in the base image as per the initial test, so I haven't added those as the websites I visited already displayed correctly. It could be the case that there are characters outside the unicode space
Looking at Playwright's deps these are the changes I see we could make:
fonts-wqy-zenhei
this seems like a more complete font set to replace the one in the package we have. However, I was just thinking about the licensing of these given that this package seems to be GPL licensed and therefore I don't know whether we could include it in the image we distribute. The Arphic Public License for the current fonts seem to only apply for modification of the fonts themselves and is incompatible with GPL and thus seems fine, but actually it would be good to review.
➡️ @andrewvc @vigneshshanmugam do we have a particular specialist in software licensing to review these?fonts-tlwg
seems to support Thai characters, so that's one I think we could add. However, it also includes GPL licensed fonts, so we must double check that.- [
fonts-noto-color-emoji
] for supporting Google's emoji set (which should really follow the same set as the standard, shouldn't it?)
I moved this PR to draft
just so that we would make sure to review the licensing and whether we're happy with the current coverage set. I do thing we should add fonts-tlwg
though, so I'll run a test for that before opening this PR for review again too.
/test |
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'm LGTM but let's address @vigneshshanmugam 's comments before merging
0291176
to
d4c483e
Compare
…iple languages [fix elastic#29495]
d4c483e
to
377ab26
Compare
After talking about this issue with @vigneshshanmugam, I've addressed a couple more of his points:
|
@lucasfcosta Thanks Lucas for testing it and providing the details.
Great.
Interesting, I didn't know the old image was not able to run headful tests. I am fine with any of the below options
Gotcha, we should make a call here. Lets see what others have to say @andrewvc @paulb-elastic @drewpost |
My gut feel is that it's OK to disable GPU acceleration. We're not a device simulation testing product. What's key is to keep the results consistent and as long as we pick one option or the other and stick with it, then that requirement is met. There may be real world impact on the sizing we need to allow for given the more inefficient CPU only approach and I'm open to that as being something we discuss from an operation efficiency standpoint but from a product perspective, I'm OK with not enabling GPU acceleration. cc @lucasfcosta @vigneshshanmugam |
This pull request is now in conflicts. Could you fix it? 🙏
|
Hey @drewpost thanks a lot for your input! 😊 With regards to your last point about GPU acceleration I've recently run some tests within containers and it seems like containers do not have access to the underlying host's GPU so there's no actual difference in performance. I've added a few more details in this link. |
I've pushed a change to use google's SIL licensed noto fonts. All the tests listed previously still work, with all languages rendering. |
…iple languages [fix elastic#29495]
…iple languages [fix elastic#29495]
6ee5938
to
1428809
Compare
I have re-tested all the websites in this PR with the new fonts and they all worked fine, including Thai websites, which I hadn't checked before. As long as everyone's happy with the chosen font's licenses, I think we can go ahead and merge this in. What do you think @andrewvc? |
/test |
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.
LGTM
1428809
to
1518c19
Compare
apt-get clean all && \ | ||
exit_code=0 && break || exit_code=$? && echo "apt-get error: retry $iter in 10s" && sleep 10; \ | ||
done; \ | ||
(exit $exit_code) | ||
RUN apt install fonts-noto-cjk |
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.
Why do we explicitly install the fonts on elastic-agent image and not on the HB image?
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.
ah, that's a bug, it's actually installed higher up, this is redundant
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.
LGTM with pending comment #29861 (comment)
72e27aa
to
668449d
Compare
…languages (#29861) This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages. After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed. (cherry picked from commit 457b0bb)
@Mergifyio backport 7.17 |
…languages (#29861) This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages. After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed. (cherry picked from commit 457b0bb)
✅ Backports have been created
|
…languages (#29861) (#30091) This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages. After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed. (cherry picked from commit 457b0bb) Co-authored-by: Lucas F. da Costa <lucas@lucasfcosta.com>
…languages (#29861) (#30093) This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages. After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed. (cherry picked from commit 457b0bb) Co-authored-by: Lucas F. da Costa <lucas@lucasfcosta.com>
…k-version-after-8-0-creation * upstream/master: (69 commits) Update stale config following (elastic#30082) Make include_matches backwards compatible with 7.x config (elastic#30032) [Filebeat] Update handling of elasticsearch server logs (elastic#30018) Remove SSL3 support from libbeat and its documentation. (elastic#30071) Revert "Packaging: rename arm64 suffix to aarch64 in the tar.gz artifacts ONLY (elastic#28813)" (elastic#30083) [libbeat] Add script processor to all beats (elastic#29752) Add fonts to support more different types of characters for multiple languages (elastic#29861) libbeat/reader: Fix messge conversion to beat.Event (elastic#30057) probot[stale]: ignore issues with the tag flaky-test (elastic#30065) [DOCS] Add redirect for GSuite module (elastic#30034) [Automation] Update elastic stack version to 8.1.0-aa69d697 for testing (elastic#30012) Remove msitools install for windows build, using the latest docker image with msitools preinstalled (elastic#30040) filebeat/generator/fields: fix dropped error (elastic#29943) Include the error message with auditd module events (elastic#30009) [Metricbeat] gcp: add firestore metricset (elastic#29918) probot: update stale dates (elastic#29997) Metricbeat enterprise search module: add xpack.enabled support (elastic#29871) x-pack/packetbeat: install Npcap at start-up when required (elastic#29112) [Filebeat] Fix panic in decode_cef when recovering from invalid data (elastic#30038) Correctly fixe how selected packages are defined (elastic#30039) ...
What does this PR do?
This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.
After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.
Therefore, this PR installs the following packages within the
elastic-agent
andheartbeat
images so that those can work fine:Why is it important?
This is important because customer's websites use a wide variety of language and we must have an inclusive synthetics product capable of displaying a wide variety of characters in the screenshots it takes.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
I'd appreciate it if reviewers could check the following items:
How to test this PR locally
For Heartbeat
heartbeat
directory withinx-pack
env PLATFORMS="+all linux/amd64" mage package
.elastic-package stack up
and then run the Heartbeat container mounting the configuration file above. That will cause the container to use the locally built image.For Elastic Agent
elastic-agent
directory withinx-pack
env PLATFORMS="+all linux/amd64" mage dev:package
.elastic-package stack up
using a compatible stack versionAn inline script like the one below for each of these websites should be enough:
elastic-agent-complete
image and change its entry-point to/bin/bash
so you can go into it.elastic-agent enroll --insecure --url=http://fleet-server:8220 --enrollment-token=POLICY_TOKEN_HERE
. You can get the enrollment token and policy tokens in Kibana (under Fleet).elastic-agent &
and wait for it to send screenshots for the monitors you've set up in Kibana.Related issues
Screenshots