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

Replace Management Samples #343

Open
wants to merge 15 commits into
base: latest
Choose a base branch
from
Open

Replace Management Samples #343

wants to merge 15 commits into from

Conversation

TimHess
Copy link
Member

@TimHess TimHess commented Oct 23, 2024

Consolidate and update Management samples

Blocked by SteeltoeOSS/Steeltoe#1389

Resolves #341

part of #319

Planned remaining items before merging:

  • Resolve gitinfo issues
  • Address Steeltoe distributed tracing vs direct OpenTelemetry setup

Additional items that could be addressed before merging or considered follow-up items:

~- [ ] List/link actuators exposed by /actuator on home page

  • Greater detail on metrics options, especially in localhost setting

@TimHess TimHess added Component/Management Issues related to Steeltoe Management (actuators) ReleaseLine/4.x labels Oct 23, 2024
@TimHess TimHess self-assigned this Oct 23, 2024
@TimHess TimHess marked this pull request as ready for review October 28, 2024 21:31
steps/browser_steps.py Show resolved Hide resolved
steps/browser_steps.py Show resolved Hide resolved
@bart-vmware

This comment was marked as resolved.

Connectors/src/CosmosDb/Program.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/README.md Outdated Show resolved Hide resolved
@bart-vmware

This comment was marked as resolved.

@bart-vmware
Copy link
Member

Following further on with the API readme steps for running locally, it suddenly stops. I've started all the docker containers and the app, now what to do next? I don't see a port number in appsettings.json. The console output contains:

Now listening on: http://[::]:5140

So I open http://localhost:5140/ in my browser, which returns a 404. Then tried the first entry from the .http file in VS, which fails with:

(1,7): error HTTP0012: Unable to evaluate expression 'HostAddress'.

I'm lost here.

@bart-vmware

This comment was marked as resolved.

@bart-vmware

This comment was marked as resolved.

Management/src/ActuatorApi/README.md Outdated Show resolved Hide resolved
@bart-vmware
Copy link
Member

bart-vmware commented Oct 30, 2024

When running the 3 tasks from readme, no forecasts are shown in browser. Then I realized I shouldn't have run the last step that clears all data. This isn't obvious at first sight. The readme(s) should provide step-by-step instructions to accomplish a scenario, such as here. It would be nice to have a per-actuator short description of what its purpose is (unless obvious), and what to expect when following the steps.

@bart-vmware
Copy link
Member

bart-vmware commented Oct 30, 2024

Navigating to SpringBootAdmin by opening http://localhost:9090 in the browser (which should be mentioned in the readme) shows Api and Web, but clicking on any of them produces an error. Then I found a commented-out line in appsettings.Development.json instructing to uncomment when using docker instead of podman. That should be mentioned in the readme as well, I wouldn't expect newcomers to dive as deeply as I did. After the change, Api appears offline in SBA and the logs from dotnet run contain multiple warnings, such as:

warn: Steeltoe.Management.Endpoint.ManagementPort.ManagementPortMiddleware[0]
      Access to /actuator/refresh on port 5140 denied because 'Management:Endpoints:Port' is set to 9999.

I get the impression that the various parts of the sample have worked at some point in time, but then things changed, without verification that every scenario still works. Right now, seeing the sample in action requires investigation, and knowledge of Steeltoe internals and the CF/Spring ecosystem, which is a blocker for adoption. The readme(s) should mention the variations in scenarios and what to do (such as changing settings or code) to make it work. I wonder if putting all features together in a single sample resulted in a highly-complex sample that defeats its purpose: showing how easy it is to use Steeltoe. How I handled that in the Discovery sample is by adding additional launch profiles to activate the advanced features, which can easily be ignored by users who don't care.

@bart-vmware
Copy link
Member

It would be nice if the home page listed all the available actuator endpoints by fetching them from /actuator and rendering clickable links (that open in a new tab).

Management/src/ActuatorApi/MySqlSeeder.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/MySqlSeeder.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/Program.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/BasicAuthExtensions.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/Properties/launchSettings.json Outdated Show resolved Hide resolved
Management/src/ActuatorWeb/Program.cs Show resolved Hide resolved
Management/src/ActuatorWeb/Program.cs Outdated Show resolved Hide resolved
git.tags=2.1.0-644-g90d0870a
git.branch=management
git.build.time=2024-10-11T13:44:28.9255701-05:00
git.build.user.name=timhe
Copy link
Member

Choose a reason for hiding this comment

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

Should this be anonymized?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a better idea... what is here now is just what was generated and I think I had to override the .gitignore for it to be included at all. There are still some oddities with whether or not this file can be generated or included in publishing that I want to better understand... like how can we get the feature to work with cf push when the git info isn't included for it to be generated (this isn't working: <Content Include="git.properties" />)

Copy link
Member

Choose a reason for hiding this comment

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

Integration with msbuild is described at https://docs.steeltoe.io/api/v3/management/info.html:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I already had that piece included, but the msbuild integration itself does not work when .git is not there (such as when source for an app that lives in a monorepo is pushed)

Copy link
Member

Choose a reason for hiding this comment

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

Just a wild guess: would it work to include the file from an MSBuild target that runs after one of the git targets, such as here?

Copy link
Member Author

Choose a reason for hiding this comment

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

File contents are anonymized and I found a very simple solution for the problem:

  <ItemGroup>
    <None Update="git.properties">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </None>
  </ItemGroup>

Copy link
Member

Choose a reason for hiding this comment

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

When I run dotnet build or dotnet publish ..., it copies the git.properties file containing some-user into the output directory. I would have expected that the MSBuild task runs and produces a git.properties file with actual data from git, like the last commit date, uid, etc. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there's a condition on the build target so it doesn't fail if the git info isn't available (eg when pushing just the app directory to CF)

<Target Name="_GitProperties" AfterTargets="CoreCompile" Condition="!Exists('git.properties')">

If you want to see what gets generated, delete the file before build/publish

Copy link
Member

Choose a reason for hiding this comment

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

Can't we make it work the other way around, ie generate the file, unless that's not possible like during cf push?

@TimHess

This comment was marked as resolved.

Copy link
Member

@bart-vmware bart-vmware left a comment

Choose a reason for hiding this comment

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

Gone over most of the files again, but haven't run anything yet.

There are quite some TODOs in the readme, can you move them to separate issues if they won't be addressed in this PR?

Management/src/ActuatorApi/WeatherEndpoints.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/AdminTasks/ForecastTask.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/AdminTasks/ForecastTask.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/BasicAuthExtensions.cs Outdated Show resolved Hide resolved
Management/src/ActuatorApi/Data/WeatherContext.cs Outdated Show resolved Hide resolved
Management/src/ActuatorWeb/Pages/Error.cshtml Show resolved Hide resolved
Management/src/ActuatorWeb/Pages/Shared/_Layout.cshtml Outdated Show resolved Hide resolved
Management/src/ActuatorWeb/Properties/launchSettings.json Outdated Show resolved Hide resolved
@@ -0,0 +1,22 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Should we put launchBrowser and launchUrl in the profiles, so the user can F5 to run the project (here and/or in api project)?

Copy link
Member

Choose a reason for hiding this comment

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

In ActuatorWeb, launchUrl should just be an empty string. In ActuatorApi, launchUrl is missing in the http profile.


<!-- TODO: more testing, more specific instructions -->

If you wish to collect and view applications metrics in [App Metrics for VMware Tanzu](https://docs.vmware.com/en/App-Metrics-for-VMware-Tanzu/index.html), you must first configure [Metrics Registrar](https://docs.pivotal.io/platform/application-service/2-9/metric-registrar/index.html) in the TAS for VMs tile. There is no separate product tile (unlike the metrics forwarder). Once thats complete custom metrics will be collected and automatically exported to the Metrics Forwarder service.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you wish to collect and view applications metrics in [App Metrics for VMware Tanzu](https://docs.vmware.com/en/App-Metrics-for-VMware-Tanzu/index.html), you must first configure [Metrics Registrar](https://docs.pivotal.io/platform/application-service/2-9/metric-registrar/index.html) in the TAS for VMs tile. There is no separate product tile (unlike the metrics forwarder). Once thats complete custom metrics will be collected and automatically exported to the Metrics Forwarder service.
If you wish to collect and view applications metrics in [App Metrics for VMware Tanzu](https://docs.vmware.com/en/App-Metrics-for-VMware-Tanzu/index.html), you must first configure [Metrics Registrar](https://docs.pivotal.io/platform/application-service/2-9/metric-registrar/index.html) in the TAS for VMs tile. There is no separate product tile (unlike the metrics forwarder). Once that's complete, custom metrics will be collected and automatically exported to the Metrics Forwarder service.

@TimHess
Copy link
Member Author

TimHess commented Nov 5, 2024

It would be nice if the home page listed all the available actuator endpoints by fetching them from /actuator and rendering clickable links (that open in a new tab).

This sounds nice and I was initially in favor of it... But the actuators are being secured with basic auth, so I don't think we can make normal clickable links. We could do some fancy JS work and build a super basic client for the actuators, but maybe we're better off relying on the .http files and/or SBA to do the heavy lifting rather than adding more.

git.tags=2.1.0-644-g90d0870a
git.branch=management
git.build.time=2024-10-11T13:44:28.9255701-05:00
git.build.user.name=timhe
Copy link
Member

Choose a reason for hiding this comment

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

When I run dotnet build or dotnet publish ..., it copies the git.properties file containing some-user into the output directory. I would have expected that the MSBuild task runs and produces a git.properties file with actual data from git, like the last commit date, uid, etc. Am I missing something?

Comment on lines +10 to +12
{
// Add OpenTelemetry
services.AddOpenTelemetry().WithTracing(tracerProviderBuilder =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
// Add OpenTelemetry
services.AddOpenTelemetry().WithTracing(tracerProviderBuilder =>
{
services.AddOpenTelemetry().WithTracing(tracerProviderBuilder =>


## Building & Running

See the Readme for instructions on building and running each app.

---

### See the Official [Steeltoe Management Documentation](https://steeltoe.io/docs/steeltoe-management) for a more in-depth walkthrough of the samples and more detailed information
### See the Official [Steeltoe Management Documentation](https://docs.steeltoe.io/api/v3/management/) for a more in-depth walkthrough of the samples and more detailed information
Copy link
Member

Choose a reason for hiding this comment

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

The markdown linter sounds right to me. I think this shouldn't be a header in the first place. It probably is today only to increase the font size. I'd be fine to remove the ### and make it a sentence. We already have separator (---) that makes it look like a footer.

README.md Show resolved Hide resolved
CommonTasks.md Outdated

```script
docker run --rm -ti -p 1433:1433 --name steeltoe-sqlserver -e 'ACCEPT_EULA=Y' -e 'SA_PASSWORD=St33ltoeR0cks!' mcr.microsoft.com/mssql/server
> Note that this image has the management plugin enabled and no credentials set:
Copy link
Member

Choose a reason for hiding this comment

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

Now that it's a note, we don't need to repeat that. And it should end with a dot, not a colon. Suggestion:

This image has the management plugin enabled and no credentials set.

@@ -0,0 +1,22 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

In ActuatorWeb, launchUrl should just be an empty string. In ActuatorApi, launchUrl is missing in the http profile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component/Management Issues related to Steeltoe Management (actuators) ReleaseLine/4.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants