-
Notifications
You must be signed in to change notification settings - Fork 58
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
Implement logs retriever pagination using new Elasticsearch Java client #315
Implement logs retriever pagination using new Elasticsearch Java client #315
Conversation
cyrille-leclerc
commented
Feb 9, 2022
- Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
- Ensure that the pull request title represents the desired changelog entry
- Please describe what you did
- Link to relevant issues in GitHub or Jira
- Link to relevant pull requests, esp. upstream and downstream changes
- Ensure you have provided tests - that demonstrates feature works or fixes the issue
.../java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageRetriever.java
Outdated
Show resolved
Hide resolved
.../java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageRetriever.java
Outdated
Show resolved
Hide resolved
.../java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageRetriever.java
Outdated
Show resolved
Hide resolved
src/test/java/io/jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchRetrieverIT.java
Outdated
Show resolved
Hide resolved
ScrollRequest scrollRequest = ScrollRequest.of(builder -> builder.scrollId(context.scrollId)); | ||
ScrollResponse<ObjectNode> scrollResponse = this.elasticsearchClient.scroll(scrollRequest, ObjectNode.class); | ||
hits = scrollResponse.hits().hits(); | ||
newScrollId = context.scrollId; // TODO why doesn't the scroll response hold a new scrollId? scrollResponse.scrollId(); |
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.
@swallez why is scrollResponse.scrollId
null
? I expected it to hold the scrollId
I should use next time so I don't have to understand if the backend has decided to reuse the sqame id or create a new one, similarly to the point in time API..
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.
Have a look at the documentation. The way scroll works is as follows:
- send a normal search request with the
scroll
parameter set that indicates a TTL, - for the next "pages", call
client.scroll()
to fetch the next batch.
hits = scrollResponse.hits().hits(); | ||
newScrollId = context.scrollId; // TODO why doesn't the scroll response hold a new scrollId? scrollResponse.scrollId(); | ||
} | ||
complete = hits.size() != PAGE_SIZE; // TODO is there smarter? |
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.
@swallez what's the best way to understand if the search request returned all the entries or if I should scroll more? as scrollResponse.scrollId
is always null
, I end up doing one wasted query if the number of entries of the response is a multiple of PAGE_SIZE
...jenkins/plugins/opentelemetry/backend/elastic/ElasticsearchLogStorageScrollingRetriever.java
Outdated
Show resolved
Hide resolved
|
I implemented log retrieval using point in time in swallez@e454add (test works AFAICT) Feel free to pull these changes here. |
thanks, can you by any chance do a PR? |
@cyrille-leclerc I'll let you take it from there, I'm happy I could help but don't have much time to dedicate to this. |
"logPosition=(end:now,start:now-1d,streamLive:!f)&" + | ||
"logFilter=(language:kuery,query:%27trace.id:" + traceId + "%27)&"; | ||
if (pageNo == 0) { | ||
w.write("View logs in Kibana: " + logsVisualizationUrl + "\n\n"); |
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.
@kuisathaverat do you know how to produce an HTML link? I guess it's related to the code of io.jenkins.plugins.opentelemetry.job.log.ConsoleNotes
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.
it depends on the render you have configured for Jenkins, my experience on showing an URL in the console log is that will work if the URL is simple, but in the case of the Kibana URL is not simple, it has many parameters with delimiters and so on, in my test I could make the link work in all the cases (probably URL encoded works better, but I did not test it), and the link is ugly because is too long. Because of that, I changed to use an Action in the previous plugin.
@kuisathaverat I don't understand how the API works, I don't succeed in having any kind of pagination. We have 3 invocations |
I think is the best approach, it limits the possibility of issues in Jenkins when you have huge logs, you always can go to Elasticsearch to check the whole log. |
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class CustomLogStorageRetriever implements LogStorageRetriever<CustomLogStorageRetriever.CustomLogsQueryContext> { |
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.
@kuisathaverat I want to introduce a generic LogStorageRetriever that just print a hyperlink to the visualization backend