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.

Sign up for GitHub

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

Added additional details to Java integration #169

Merged
merged 5 commits into from
Apr 10, 2023
Merged

Added additional details to Java integration #169

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abae1f3
Added additional details to Java integration
6fears7 Apr 10, 2023
e29df7b
Updated with suggested fixes
6fears7 Apr 10, 2023
854f2db
Updated with suggested fixes
6fears7 Apr 10, 2023
b9443dc
Adds tabgroups to java documentation page
Rperry2174 Apr 10, 2023
12b50a7
Merge branch 'update/java/springboot-setup' of https://github.com/6fe…
Rperry2174 Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions docs/integration-java.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

6fears7 marked this conversation as resolved.
Show resolved Hide resolved
---
id: integration-java
title: Java
Expand Down Expand Up @@ -43,18 +44,40 @@ Add pyroscope dependency:
```kotlin
implementation("io.pyroscope:agent:${pyroscope_version}")
```
As .xml:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think this is very useful, there are other build systems like sbt,ivy, etc, and we are not going to provide a snippet for each of them. The gradle snippet provides enough bits to be able to add it to other build systems. Also I beilieve intellij idea is able to convert it back and forth automatically when you paste a dependency to a build script.

I think there should be just one example and between gradle and maven I prefer gradle since its more concise.

Copy link
Member

Choose a reason for hiding this comment

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

@korniltsev what do you think if we do a tabgroup and over time people can update docs accordingly:
image
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good. This should do the job

nit: in your example tab with title "maven" contains gradle example

btw here is what maven central does (which we have a link to, just above the section)
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

And what about other build systems? Should we add them as well?

Copy link
Member

Choose a reason for hiding this comment

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

I think eventually we will have other helpful community members like @6fears7 to help us add those if they feel they are necessary :)

```kotlin
<dependency>
<groupId>io.pyroscope</groupId>
<artifactId>agent</artifactId>
<version>pyroscope_version</version>
</dependency>
```

Then add the following code to your application:
```java

import io.pyroscope.javaagent.PyroscopeAgent;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think imports are that important. They make an example more verbose while not providing much value. Imports are also automatically added with modern smart IDE.
Although maybe we should still add imports for consistency(golang, python etc have import)

Copy link
Member

Choose a reason for hiding this comment

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

yeah I agree it makes it a little verbose, but let's leave this in for consistency in this case.

import io.pyroscope.javaagent.config.Config;
import io.pyroscope.javaagent.EventType;
import io.pyroscope.http.Format;



@PostConstruct
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why spring? What about Vaadin, Spark, Grails, Play
Framework example should not be the default one. The default one should be just plain java.
Maybe we could add a spring notice somewhere down the page, like "Hey if you use spring, you can put this initialization in PostConstruct". At the same time I am not sure it should go to PostConstruct, I am not that familiar with spring but it looks like it will be invoked somewhere in the middle of bean graph initialization, and I believe the best moment to start profiling is before the whole initialization(somewhere in main or clinit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll default to your judgment. I do not primarily develop in Java and was going through setting up an example Spring Boot application for our Dev teams using Maven and had to spend some time working through what the docs implied vs declared and found these to be useful. I prefer exhaustive documentation that covers multiple popular frameworks, but that is a preference of mine obviously and not a standard

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be resolved with tabgroups see example here

I agree the default should be plain java, but I think with a tabgroup we can show the version that works with spring

Copy link
Collaborator

Choose a reason for hiding this comment

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

And what about other frameworks? Should we add them as well?

Copy link
Member

Choose a reason for hiding this comment

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

I'll create an issue for it, but definitely not as part of this pr. I think if others want to add them we will appreciate their help similar to @6fears7

public void init() {

PyroscopeAgent.start(
new Config.Builder()
.setApplicationName("ride-sharing-app-java")
.setProfilingEvent(EventType.ITIMER)
.setFormat(Format.JFR)
.setProfilingAlloc("0") // sets allocation threshold to register events, in bytes. '0' registers all events
6fears7 marked this conversation as resolved.
Show resolved Hide resolved
6fears7 marked this conversation as resolved.
Show resolved Hide resolved
.setServerAddress("http://pyroscope-server:4040")
// Optionally, if authentication is enabled, specify the API key.
// .setAuthToken(System.getenv("PYROSCOPE_AUTH_TOKEN"))
.build()
);
}
```

You can also optionally replace some pyroscope components
Expand Down