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

Azure Resource Metrics - Apply doc template and add generic setup section #9065

Merged
merged 13 commits into from
Feb 19, 2024

Conversation

alaudazzi
Copy link
Contributor

@alaudazzi alaudazzi commented Feb 6, 2024

This PR:

Relates #123

@alaudazzi alaudazzi added documentation Improvements or additions to documentation draft Draft labels Feb 6, 2024
@alaudazzi alaudazzi requested a review from zmoog February 6, 2024 11:08
@elasticmachine
Copy link

elasticmachine commented Feb 6, 2024

🚀 Benchmarks report

Package azure_metrics 👍(2) 💚(1) 💔(1)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
container_service 200000 142857.14 -57142.86 (-28.57%) 💔

To see the full report comment with /test benchmark fullreport

@alaudazzi
Copy link
Contributor Author

alaudazzi commented Feb 7, 2024

@zmoog
Here is a doc preview.

Having the Exported fields table following each type of data streams makes this page not readable. I can test the collapse/expand feature in MD and see how it renders in MDX, but if it doesn't work I suggest we move the Exported fields table at the end of the page, just before the Changelog section. WDYT?

packages/azure_metrics/_dev/build/docs/README.md Outdated Show resolved Hide resolved
packages/azure_metrics/_dev/build/docs/README.md Outdated Show resolved Hide resolved
packages/azure_metrics/_dev/build/docs/README.md Outdated Show resolved Hide resolved
packages/azure_metrics/_dev/build/docs/README.md Outdated Show resolved Hide resolved

`monitor`
`monitor`
Copy link
Contributor

Choose a reason for hiding this comment

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

After reading the preview, I have the feeling that we should move this section with the metricset description and tables (generaged by {{fields "monitor"}}) at the bottom of the doc in a "references" section.

The {{fields "monitor"}} expands in a large table, and IMO it's easy to miss the content after the tables.

What about adding something in the lines of "see references section for all the gory details of these metricsets" right after the metricset list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 👍 that's what I also suggested here. If the test with collapse/expand feature doesn't work, we'll put the tables at the end of the page and reference them where needed.

alaudazzi and others added 4 commits February 9, 2024 10:38
Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
@alaudazzi
Copy link
Contributor Author

alaudazzi commented Feb 10, 2024

@zmoog
Consider some further optimization on this page:

The first block of text of Integration specific configuration notes and the entire section of Additional notes about metrics and costs can be combined into a single and more linear section called Authentication and costs.

For example:


image

If agreed, this change would be reflected on all these pages:

@zmoog
Copy link
Contributor

zmoog commented Feb 12, 2024

The first block of text of Integration specific configuration notes and the entire section of Additional notes about metrics and costs can be combined into a single and more linear section called Authentication and costs.

Yep, it makes senso combine these two sections into one.

Two notes on the content:

  • In all our Azure-related docs, we always use the term "Azure AD" (or "Azure Active Directory"). It seems that Microsoft renamed Azure AD in Microsoft Entra ID. Updating these names in all docs will probably require a dedicated PR, so I don't know if we can start updating the portions we change in this one.
  • In these section, we use the term "service principal". IIRC, this is a different name for the "app registration" concept. We need to double-check if "service principal" and "app registration" are the same thing or not and decide how to deal with them.

@zmoog
Copy link
Contributor

zmoog commented Feb 12, 2024

If we want to continue using the ASCII art for diagrams, I can share the text for azure_resource_metrics_req.png.

@zmoog
Copy link
Contributor

zmoog commented Feb 12, 2024

In these section, we use the term "service principal". IIRC, this is a different name for the "app registration" concept. We need to double-check if "service principal" and "app registration" are the same thing or not and decide how to deal with them.

The name "service principal" is commonly used to represent the identity of an application for authentication purposes. Other cloud providers, for example GCP, use the term service principal for this concept.

On Azure, they seem to use "app registration" on the Portal, and "service principal" on the CLI and API:

On the Azure CLI, if I list the service principals:

$ az ad sp list | jq -r '.[] | [.appDisplayName] | @tsv' | grep branca

mbranca-app-registration-or-service-principal
mbranca-elastic-agent

I get the same result if I open the app registrations section on the Portal:

CleanShot 2024-02-12 at 17 46 55@2x

@alaudazzi
Copy link
Contributor Author

alaudazzi commented Feb 13, 2024

@zmoog

On Azure, they seem to use "app registration" on the Portal, and "service principal" on the CLI and API:

So, the concept is the same, the only difference is that app registration is on the UI and service principal is on the CLI?

Microsoft renamed Azure AD in Microsoft Entra

We fix the occurrences in this PR and handle the remaining ones in a separate PR

@zmoog
Copy link
Contributor

zmoog commented Feb 13, 2024

@alaudazzi, here is the ASCII version of the diagram:

                       ┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┐

                       │  ┌─────────────────┐  │
┌─────────────────┐       │  azure-monitor  │       ┌─────────────────┐
│    Azure API    │◀───┼──│  <<metricset>>  │──┼───▶│  Elasticsearch  │
└─────────────────┘       └─────────────────┘       └─────────────────┘
                       │                       │
                        ─ Elastic Agent ─ ─ ─ ─

@zmoog
Copy link
Contributor

zmoog commented Feb 13, 2024

The name "service principal" is commonly used to represent the identity of an application for authentication purposes. Other cloud providers, for example GCP, use the term service principal for this concept.

It turns out that on Azure "app registration" and "service principal" are related but NOT the same.

Quoting Create a Microsoft Entra application and service principal that can access resources from the Azure docs:

When you register a new application in Microsoft Entra ID, a service principal is automatically created for the app registration. The service principal is the app's identity in the Microsoft Entra tenant.

So, "app registration" and "service principal" are different entities. Azure creates the related service principal for you if you create an app registration. And it works both ways!

So users have the option of creating:

  • (A) an app registration on the Azure Portal (probably easier to do manually), and get the service principal for free.
  • (B) a service registration using the CLI or the API (probably more accessible to automate), and get an app registration for free.

@alaudazzi alaudazzi marked this pull request as ready for review February 15, 2024 09:40
@alaudazzi alaudazzi requested a review from a team as a code owner February 15, 2024 09:40
@zmoog
Copy link
Contributor

zmoog commented Feb 15, 2024

I would use the app registration in our docs to resolve the "app registration" vs. "service principal" dilemma. The App registration is the most visible term in Azure Portal, the UI we primarily use in our docs.

Then, if we link to Azure docs that explain how to create a service principal using the CLI (bash or PowerShell), in that case, we can mention that creating a service principal will also create a linked app registration that will be visible on the Portal.

@alaudazzi, WDYT?

@alaudazzi
Copy link
Contributor Author

alaudazzi commented Feb 19, 2024

@zmoog

I would use the app registration in our docs to resolve the "app registration" vs. "service principal" dilemma. The App registration is the most visible term in Azure Portal, the UI we primarily use in our docs.

makes sense

Then, if we link to Azure docs that explain how to create a service principal using the CLI (bash or PowerShell), in that case, we can mention that creating a service principal will also create a linked app registration that will be visible on the Portal.

I added a note in the Authentication and costs section.

The PR is ready for you to review. I guess we integrated all the comments raised so far.

Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

LGTM!

nit: I think we can remove

Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

LGTM!

Nit: I think we can remove the .png file since we opted to use the ASCII version of the diagram.

@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

@alaudazzi alaudazzi merged commit 0230540 into main Feb 19, 2024
5 checks passed
@alaudazzi alaudazzi added enhancement New feature or request and removed draft Draft labels Feb 20, 2024
alaudazzi added a commit that referenced this pull request Feb 20, 2024
alaudazzi added a commit that referenced this pull request Feb 20, 2024
* Update changelog and manifest for #9065

* Update changelog.yml
@elasticmachine
Copy link

Package azure_metrics - 1.4.2 containing this change is available at https://epr.elastic.co/search?package=azure_metrics

gizas pushed a commit that referenced this pull request Mar 13, 2024
…tion (#9065)

* Apply doc template and add generic setup section

* Update packages/azure_metrics/_dev/build/docs/README.md

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

* Update packages/azure_metrics/_dev/build/docs/README.md

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

* Update packages/azure_metrics/_dev/build/docs/README.md

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

* Update packages/azure_metrics/_dev/build/docs/README.md

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>

* Move the data streams to a reference section

* Fix data streams wording

* Optimize config auth costs section

* Replace MS AD with MS Entra

* Replace Active Directory with MS Entra

* Add diagram

* Add note on service principal

* Remove unused image

---------

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
gizas pushed a commit that referenced this pull request Mar 13, 2024
* Update changelog and manifest for #9065

* Update changelog.yml
@andrewkroh andrewkroh added the Integration:azure_metrics Azure Resource Metrics label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request Integration:azure_metrics Azure Resource Metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants